Improve email parsing

Closes #212289

app/models/x509_issuer.rb

@@ -7,7 +7,7 @@ class X509Issuer < ApplicationRecord
   validates :subject_key_identifier, presence: true, format: { with: /\A(\h{2}:){19}\h{2}\z/ }
   # rfc 5280 - 4.1.2.4  Issuer
   validates :subject, presence: true
-  # rfc 5280 - 4.2.1.14  CRL Distribution Points
+  # rfc 5280 - 4.2.1.13  CRL Distribution Points
   # cRLDistributionPoints extension using URI:http
   validates :crl_url, presence: true, public_url: true
 

changelogs/unreleased/refactor-x509-commit-to-signature.yml

+---
+title: Extract X509::Signature from X509::Commit
+merge_request: 27327
+author: Roger Meier
+type: changed

lib/gitlab/x509/commit.rb

@@ -31,175 +31,23 @@ module Gitlab
         end
       end
 
-      def verified_signature
-        strong_memoize(:verified_signature) { verified_signature? }
-      end
-
-      def cert
-        strong_memoize(:cert) do
-          signer_certificate(p7) if valid_signature?
-        end
-      end
-
-      def cert_store
-        strong_memoize(:cert_store) do
-          store = OpenSSL::X509::Store.new
-          store.set_default_paths
-          # valid_signing_time? checks the time attributes already
-          # this flag is required, otherwise expired certificates would become
-          # unverified when notAfter within certificate attribute is reached
-          store.flags = OpenSSL::X509::V_FLAG_NO_CHECK_TIME
-          store
-        end
-      end
-
-      def p7
-        strong_memoize(:p7) do
-          pkcs7_text = signature_text.sub('-----BEGIN SIGNED MESSAGE-----', '-----BEGIN PKCS7-----')
-          pkcs7_text = pkcs7_text.sub('-----END SIGNED MESSAGE-----', '-----END PKCS7-----')
-
-          OpenSSL::PKCS7.new(pkcs7_text)
-        rescue
-          nil
-        end
-      end
-
-      def valid_signing_time?
-        # rfc 5280 - 4.1.2.5  Validity
-        # check if signed_time is within the time range (notBefore/notAfter)
-        # non-rfc - git specific check: signed_time >= commit_time
-        p7.signers[0].signed_time.between?(cert.not_before, cert.not_after) &&
-          p7.signers[0].signed_time >= @commit.created_at
-      end
-
-      def valid_signature?
-        p7.verify([], cert_store, signed_text, OpenSSL::PKCS7::NOVERIFY)
-      rescue
-        nil
-      end
-
-      def verified_signature?
-        # verify has multiple options but only a boolean return value
-        # so first verify without certificate chain
-        if valid_signature?
-          if valid_signing_time?
-            # verify with system certificate chain
-            p7.verify([], cert_store, signed_text)
-          else
-            false
-          end
-        else
-          nil
-        end
-      rescue
-        nil
-      end
-
-      def signer_certificate(p7)
-        p7.certificates.each do |cert|
-          next if cert.serial != p7.signers[0].serial
-
-          return cert
-        end
-      end
-
-      def certificate_crl
-        extension = get_certificate_extension('crlDistributionPoints')
-        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")
-              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
-            return ext.value
-          end
-        end
-      end
-
-      def issuer_subject_key_identifier
-        get_certificate_extension('authorityKeyIdentifier').gsub("keyid:", "").delete!("\n")
-      end
-
-      def certificate_subject_key_identifier
-        get_certificate_extension('subjectKeyIdentifier')
-      end
-
-      def certificate_issuer
-        cert.issuer.to_s(OpenSSL::X509::Name::RFC2253)
-      end
-
-      def certificate_subject
-        cert.subject.to_s(OpenSSL::X509::Name::RFC2253)
-      end
-
-      def certificate_email
-        get_certificate_extension('subjectAltName').split('email:')[1]
-      end
-
-      def issuer_attributes
-        return if verified_signature.nil?
-
-        {
-          subject_key_identifier: issuer_subject_key_identifier,
-          subject: certificate_issuer,
-          crl_url: certificate_crl
-        }
-      end
-
-      def certificate_attributes
-        return if verified_signature.nil?
-
-        issuer = X509Issuer.safe_create!(issuer_attributes)
-
-        {
-          subject_key_identifier: certificate_subject_key_identifier,
-          subject: certificate_subject,
-          email: certificate_email,
-          serial_number: cert.serial,
-          x509_issuer_id: issuer.id
-        }
-      end
-
       def attributes
-        return if verified_signature.nil?
+        return if @commit.sha.nil? || @commit.project.nil?
 
-        certificate = X509Certificate.safe_create!(certificate_attributes)
+        signature = X509::Signature.new(signature_text, signed_text, @commit.committer_email, @commit.created_at)
+
+        return if signature.verified_signature.nil? || signature.x509_certificate.nil?
 
         {
           commit_sha: @commit.sha,
           project: @commit.project,
-          x509_certificate_id: certificate.id,
-          verification_status: verification_status(certificate)
+          x509_certificate_id: signature.x509_certificate.id,
+          verification_status: signature.verification_status
         }
       end
 
-      def verification_status(certificate)
-        return :unverified if certificate.revoked?
-
-        if verified_signature && certificate_email == @commit.committer_email
-          :verified
-        else
-          :unverified
-        end
-      end
-
       def create_cached_signature!
-        return if verified_signature.nil?
+        return if attributes.nil?
 
         return X509CommitSignature.new(attributes) if Gitlab::Database.read_only?
 

lib/gitlab/x509/signature.rb

+# frozen_string_literal: true
+require 'openssl'
+require 'digest'
+
+module Gitlab
+  module X509
+    class Signature
+      include Gitlab::Utils::StrongMemoize
+
+      attr_reader :signature_text, :signed_text, :created_at
+
+      def initialize(signature_text, signed_text, email, created_at)
+        @signature_text = signature_text
+        @signed_text = signed_text
+        @email = email
+        @created_at = created_at
+      end
+
+      def x509_certificate
+        return if certificate_attributes.nil?
+
+        X509Certificate.safe_create!(certificate_attributes) unless verified_signature.nil?
+      end
+
+      def verified_signature
+        strong_memoize(:verified_signature) { verified_signature? }
+      end
+
+      def verification_status
+        return :unverified if x509_certificate.nil? || x509_certificate.revoked?
+
+        if verified_signature && certificate_email == @email
+          :verified
+        else
+          :unverified
+        end
+      end
+
+      private
+
+      def cert
+        strong_memoize(:cert) do
+          signer_certificate(p7) if valid_signature?
+        end
+      end
+
+      def cert_store
+        strong_memoize(:cert_store) do
+          store = OpenSSL::X509::Store.new
+          store.set_default_paths
+          # valid_signing_time? checks the time attributes already
+          # this flag is required, otherwise expired certificates would become
+          # unverified when notAfter within certificate attribute is reached
+          store.flags = OpenSSL::X509::V_FLAG_NO_CHECK_TIME
+          store
+        end
+      end
+
+      def p7
+        strong_memoize(:p7) do
+          pkcs7_text = signature_text.sub('-----BEGIN SIGNED MESSAGE-----', '-----BEGIN PKCS7-----')
+          pkcs7_text = pkcs7_text.sub('-----END SIGNED MESSAGE-----', '-----END PKCS7-----')
+
+          OpenSSL::PKCS7.new(pkcs7_text)
+        rescue
+          nil
+        end
+      end
+
+      def valid_signing_time?
+        # rfc 5280 - 4.1.2.5  Validity
+        # check if signed_time is within the time range (notBefore/notAfter)
+        # non-rfc - git specific check: signed_time >= commit_time
+        p7.signers[0].signed_time.between?(cert.not_before, cert.not_after) &&
+          p7.signers[0].signed_time >= created_at
+      end
+
+      def valid_signature?
+        p7.verify([], cert_store, signed_text, OpenSSL::PKCS7::NOVERIFY)
+      rescue
+        nil
+      end
+
+      def verified_signature?
+        # verify has multiple options but only a boolean return value
+        # so first verify without certificate chain
+        if valid_signature?
+          if valid_signing_time?
+            # verify with system certificate chain
+            p7.verify([], cert_store, signed_text)
+          else
+            false
+          end
+        else
+          nil
+        end
+      rescue
+        nil
+      end
+
+      def signer_certificate(p7)
+        p7.certificates.each do |cert|
+          next if cert.serial != p7.signers[0].serial
+
+          return cert
+        end
+      end
+
+      def certificate_crl
+        extension = get_certificate_extension('crlDistributionPoints')
+        return if extension.nil?
+
+        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")
+              crl_url = item.strip
+              break
+            end
+          end
+        end
+
+        crl_url
+      end
+
+      def get_certificate_extension(extension)
+        ext = cert.extensions.detect { |ext| ext.oid == extension }
+        ext&.value
+      end
+
+      def issuer_subject_key_identifier
+        key_identifier = get_certificate_extension('authorityKeyIdentifier')
+        return if key_identifier.nil?
+
+        key_identifier.gsub("keyid:", "").delete!("\n")
+      end
+
+      def certificate_subject_key_identifier
+        key_identifier = get_certificate_extension('subjectKeyIdentifier')
+        return if key_identifier.nil?
+
+        key_identifier
+      end
+
+      def certificate_issuer
+        cert.issuer.to_s(OpenSSL::X509::Name::RFC2253)
+      end
+
+      def certificate_subject
+        cert.subject.to_s(OpenSSL::X509::Name::RFC2253)
+      end
+
+      def certificate_email
+        email = nil
+
+        get_certificate_extension('subjectAltName').split(',').each do |item|
+          if item.strip.start_with?("email")
+            email = item.split('email:')[1]
+            break
+          end
+        end
+
+        return if email.nil?
+
+        email
+      end
+
+      def x509_issuer
+        return if verified_signature.nil? || issuer_subject_key_identifier.nil? || certificate_crl.nil?
+
+        attributes = {
+          subject_key_identifier: issuer_subject_key_identifier,
+          subject: certificate_issuer,
+          crl_url: certificate_crl
+        }
+
+        X509Issuer.safe_create!(attributes) unless verified_signature.nil?
+      end
+
+      def certificate_attributes
+        return if verified_signature.nil? || certificate_subject_key_identifier.nil? || x509_issuer.nil?
+
+        {
+          subject_key_identifier: certificate_subject_key_identifier,
+          subject: certificate_subject,
+          email: certificate_email,
+          serial_number: cert.serial.to_i,
+          x509_issuer_id: x509_issuer.id
+        }
+      end
+    end
+  end
+end

spec/lib/gitlab/x509/signature_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Gitlab::X509::Signature do
+  let(:issuer_attributes) do
+    {
+      subject_key_identifier: X509Helpers::User1.issuer_subject_key_identifier,
+      subject: X509Helpers::User1.certificate_issuer,
+      crl_url: X509Helpers::User1.certificate_crl
+    }
+  end
+
+  context 'commit signature' do
+    let(:certificate_attributes) do
+      {
+        subject_key_identifier: X509Helpers::User1.certificate_subject_key_identifier,
+        subject: X509Helpers::User1.certificate_subject,
+        email: X509Helpers::User1.certificate_email,
+        serial_number: X509Helpers::User1.certificate_serial
+      }
+    end
+
+    context 'verified signature' do
+      context 'with trusted certificate store' do
+        before do
+          store = OpenSSL::X509::Store.new
+          certificate = OpenSSL::X509::Certificate.new(X509Helpers::User1.trust_cert)
+          store.add_cert(certificate)
+          allow(OpenSSL::X509::Store).to receive(:new).and_return(store)
+        end
+
+        it 'returns a verified signature if email does match' do
+          signature = described_class.new(
+            X509Helpers::User1.signed_commit_signature,
+            X509Helpers::User1.signed_commit_base_data,
+            X509Helpers::User1.certificate_email,
+            X509Helpers::User1.signed_commit_time
+          )
+
+          expect(signature.x509_certificate).to have_attributes(certificate_attributes)
+          expect(signature.x509_certificate.x509_issuer).to have_attributes(issuer_attributes)
+          expect(signature.verified_signature).to be_truthy
+          expect(signature.verification_status).to eq(:verified)
+        end
+
+        it 'returns an unverified signature if email does not match' do
+          signature = described_class.new(
+            X509Helpers::User1.signed_commit_signature,
+            X509Helpers::User1.signed_commit_base_data,
+            "gitlab@example.com",
+            X509Helpers::User1.signed_commit_time
+          )
+
+          expect(signature.x509_certificate).to have_attributes(certificate_attributes)
+          expect(signature.x509_certificate.x509_issuer).to have_attributes(issuer_attributes)
+          expect(signature.verified_signature).to be_truthy
+          expect(signature.verification_status).to eq(:unverified)
+        end
+
+        it 'returns an unverified signature if email does match and time is wrong' do
+          signature = described_class.new(
+            X509Helpers::User1.signed_commit_signature,
+            X509Helpers::User1.signed_commit_base_data,
+            X509Helpers::User1.certificate_email,
+            Time.new(2020, 2, 22)
+          )
+
+          expect(signature.x509_certificate).to have_attributes(certificate_attributes)
+          expect(signature.x509_certificate.x509_issuer).to have_attributes(issuer_attributes)
+          expect(signature.verified_signature).to be_falsey
+          expect(signature.verification_status).to eq(:unverified)
+        end
+
+        it 'returns an unverified signature if certificate is revoked' do
+          signature = described_class.new(
+            X509Helpers::User1.signed_commit_signature,
+            X509Helpers::User1.signed_commit_base_data,
+            X509Helpers::User1.certificate_email,
+            X509Helpers::User1.signed_commit_time
+          )
+
+          expect(signature.verification_status).to eq(:verified)
+
+          signature.x509_certificate.revoked!
+
+          expect(signature.verification_status).to eq(:unverified)
+        end
+      end
+
+      context 'without trusted certificate within store' do
+        before do
+          store = OpenSSL::X509::Store.new
+          allow(OpenSSL::X509::Store).to receive(:new)
+              .and_return(
+                store
+              )
+        end
+
+        it 'returns an unverified signature' do
+          signature = described_class.new(
+            X509Helpers::User1.signed_commit_signature,
+            X509Helpers::User1.signed_commit_base_data,
+            X509Helpers::User1.certificate_email,
+            X509Helpers::User1.signed_commit_time
+          )
+
+          expect(signature.x509_certificate).to have_attributes(certificate_attributes)
+          expect(signature.x509_certificate.x509_issuer).to have_attributes(issuer_attributes)
+          expect(signature.verified_signature).to be_falsey
+          expect(signature.verification_status).to eq(:unverified)
+        end
+      end
+    end
+
+    context 'invalid signature' do
+      it 'returns nil' do
+        signature = described_class.new(
+          X509Helpers::User1.signed_commit_signature.tr('A', 'B'),
+          X509Helpers::User1.signed_commit_base_data,
+          X509Helpers::User1.certificate_email,
+          X509Helpers::User1.signed_commit_time
+        )
+        expect(signature.x509_certificate).to be_nil
+        expect(signature.verified_signature).to be_falsey
+        expect(signature.verification_status).to eq(:unverified)
+      end
+    end
+
+    context 'invalid commit message' do
+      it 'returns nil' do
+        signature = described_class.new(
+          X509Helpers::User1.signed_commit_signature,
+          'x',
+          X509Helpers::User1.certificate_email,
+          X509Helpers::User1.signed_commit_time
+        )
+        expect(signature.x509_certificate).to be_nil
+        expect(signature.verified_signature).to be_falsey
+        expect(signature.verification_status).to eq(:unverified)
+      end
+    end
+  end
+
+  context 'certificate_crl' do
+    describe 'valid crlDistributionPoints' do
+      before do
+        allow_any_instance_of(Gitlab::X509::Signature).to receive(:get_certificate_extension).and_call_original
+
+        allow_any_instance_of(Gitlab::X509::Signature).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 'creates an issuer' do
+        signature = described_class.new(
+          X509Helpers::User1.signed_commit_signature,
+          X509Helpers::User1.signed_commit_base_data,
+          X509Helpers::User1.certificate_email,
+          X509Helpers::User1.signed_commit_time
+        )
+
+        expect(signature.x509_certificate.x509_issuer).to have_attributes(issuer_attributes)
+      end
+    end
+
+    describe 'valid crlDistributionPoints providing multiple http URIs' do
+      before do
+        allow_any_instance_of(Gitlab::X509::Signature).to receive(:get_certificate_extension).and_call_original
+
+        allow_any_instance_of(Gitlab::X509::Signature).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
+        signature = described_class.new(
+          X509Helpers::User1.signed_commit_signature,
+          X509Helpers::User1.signed_commit_base_data,
+          X509Helpers::User1.certificate_email,
+          X509Helpers::User1.signed_commit_time
+        )
+
+        expect(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
+
+  context 'email' do
+    describe 'subjectAltName with email, othername' do
+      before do
+        allow_any_instance_of(Gitlab::X509::Signature).to receive(:get_certificate_extension).and_call_original
+
+        allow_any_instance_of(Gitlab::X509::Signature).to receive(:get_certificate_extension)
+          .with('subjectAltName')
+          .and_return("email:gitlab@example.com, othername:<unsupported>")
+      end
+
+      it 'extracts email' do
+        signature = described_class.new(
+          X509Helpers::User1.signed_commit_signature,
+          X509Helpers::User1.signed_commit_base_data,
+          'gitlab@example.com',
+          X509Helpers::User1.signed_commit_time
+        )
+
+        expect(signature.x509_certificate.email).to eq("gitlab@example.com")
+      end
+    end
+
+    describe 'subjectAltName with othername, email' do
+      before do
+        allow_any_instance_of(Gitlab::X509::Signature).to receive(:get_certificate_extension).and_call_original
+
+        allow_any_instance_of(Gitlab::X509::Signature).to receive(:get_certificate_extension)
+          .with('subjectAltName')
+          .and_return("othername:<unsupported>, email:gitlab@example.com")
+      end
+
+      it 'extracts email' do
+        signature = described_class.new(
+          X509Helpers::User1.signed_commit_signature,
+          X509Helpers::User1.signed_commit_base_data,
+          'gitlab@example.com',
+          X509Helpers::User1.signed_commit_time
+        )
+
+        expect(signature.x509_certificate.email).to eq("gitlab@example.com")
+      end
+    end
+  end
+end

spec/support/helpers/x509_helpers.rb

@@ -169,6 +169,10 @@ module X509Helpers
       SIGNEDDATA
     end
 
+    def signed_commit_time
+      Time.at(1561027326)
+    end
+
     def certificate_crl
       'http://ch.siemens.com/pki?ZZZZZZA2.crl'
     end