Merge branch '25616_reject_multiple_signatures' into 'master'

Reject multiple PGP signatures for commits

See merge request gitlab-org/gitlab!74095

app/models/gpg_signature.rb

@@ -12,7 +12,8 @@ class GpgSignature < ApplicationRecord
     same_user_different_email: 2,
     other_user: 3,
     unverified_key: 4,
-    unknown_key: 5
+    unknown_key: 5,
+    multiple_signatures: 6
   }
 
   belongs_to :project

app/views/projects/commit/_multiple_signatures_signature_badge.html.haml

+- title = capture do
+  = html_escape(_('This commit was signed with %{strong_open}multiple%{strong_close} signatures.')) % { strong_open: '<strong>'.html_safe, strong_close: '</strong>'.html_safe }
+
+- locals = { signature: signature, title: title, label: _('Unverified'), css_class: 'invalid', icon: 'status_notfound_borderless' }
+
+= render partial: 'projects/commit/signature_badge', locals: locals

config/feature_flags/development/multiple_gpg_signatures.yml

+---
+name: multiple_gpg_signatures
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/74095
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/345261
+milestone: '14.5'
+type: development
+group: group::source code
+default_enabled: false

lib/gitlab/gpg/commit.rb

@@ -48,7 +48,7 @@ module Gitlab
 
           if gpg_key
             Gitlab::Gpg::CurrentKeyChain.add(gpg_key.key)
-            clear_memoization(:verified_signature)
+            clear_memoization(:gpg_signatures)
           end
 
           yield gpg_key
@@ -56,16 +56,7 @@ module Gitlab
       end
 
       def verified_signature
-        strong_memoize(:verified_signature) { gpgme_signature }
-      end
-
-      def gpgme_signature
-        GPGME::Crypto.new.verify(signature_text, signed_text: signed_text) do |verified_signature|
-          # Return the first signature for now: https://gitlab.com/gitlab-org/gitlab-foss/issues/54932
-          break verified_signature
-        end
-      rescue GPGME::Error
-        nil
+        gpg_signatures.first
       end
 
       def create_cached_signature!
@@ -77,6 +68,24 @@ module Gitlab
         end
       end
 
+      def gpg_signatures
+        strong_memoize(:gpg_signatures) do
+          signatures = []
+
+          GPGME::Crypto.new.verify(signature_text, signed_text: signed_text) do |verified_signature|
+            signatures << verified_signature
+          end
+
+          signatures
+        rescue GPGME::Error
+          []
+        end
+      end
+
+      def multiple_signatures?
+        gpg_signatures.size > 1
+      end
+
       def attributes(gpg_key)
         user_infos = user_infos(gpg_key)
         verification_status = verification_status(gpg_key)
@@ -93,6 +102,7 @@ module Gitlab
       end
 
       def verification_status(gpg_key)
+        return :multiple_signatures if multiple_signatures? && Feature.enabled?(:multiple_gpg_signatures, @commit.project, default_enabled: :yaml)
         return :unknown_key unless gpg_key
         return :unverified_key unless gpg_key.verified?
         return :unverified unless verified_signature&.valid?

locale/gitlab.pot

@@ -35112,6 +35112,9 @@ msgstr ""
 msgid "This commit is part of merge request %{link_to_merge_request}. Comments created here will be created in the context of that merge request."
 msgstr ""
 
+msgid "This commit was signed with %{strong_open}multiple%{strong_close} signatures."
+msgstr ""
+
 msgid "This commit was signed with a %{strong_open}verified%{strong_close} signature and the committer email is verified to belong to the same user."
 msgstr ""
 

spec/features/signed_commits_spec.rb

@@ -115,6 +115,19 @@ RSpec.describe 'GPG signed commits' do
       end
     end
 
+    it 'unverified signature: commit contains multiple GPG signatures' do
+      user_1_key
+
+      visit project_commit_path(project, GpgHelpers::MULTIPLE_SIGNATURES_SHA)
+      wait_for_all_requests
+
+      page.find('.gpg-status-box', text: 'Unverified').click
+
+      within '.popover' do
+        expect(page).to have_content "This commit was signed with multiple signatures."
+      end
+    end
+
     it 'verified and the gpg user has a gitlab profile' do
       user_1_key
 
@@ -169,7 +182,7 @@ RSpec.describe 'GPG signed commits' do
         page.find('.gpg-status-box', text: 'Unverified').click
 
         within '.popover' do
-          expect(page).to have_content 'This commit was signed with an unverified signature'
+          expect(page).to have_content 'This commit was signed with multiple signatures.'
         end
       end
     end

spec/lib/gitlab/gpg/commit_spec.rb

@@ -136,7 +136,7 @@ RSpec.describe Gitlab::Gpg::Commit do
           it 'returns a valid signature' do
             verified_signature = double('verified-signature', fingerprint: GpgHelpers::User1.fingerprint, valid?: true)
             allow(GPGME::Crypto).to receive(:new).and_return(crypto)
-            allow(crypto).to receive(:verify).and_return(verified_signature)
+            allow(crypto).to receive(:verify).and_yield(verified_signature)
 
             signature = described_class.new(commit).signature
 
@@ -178,7 +178,7 @@ RSpec.describe Gitlab::Gpg::Commit do
             keyid = GpgHelpers::User1.fingerprint.last(16)
             verified_signature = double('verified-signature', fingerprint: keyid, valid?: true)
             allow(GPGME::Crypto).to receive(:new).and_return(crypto)
-            allow(crypto).to receive(:verify).and_return(verified_signature)
+            allow(crypto).to receive(:verify).and_yield(verified_signature)
 
             signature = described_class.new(commit).signature
 
@@ -194,6 +194,71 @@ RSpec.describe Gitlab::Gpg::Commit do
           end
         end
 
+        context 'commit with multiple signatures' do
+          let!(:commit) { create :commit, project: project, sha: commit_sha, committer_email: GpgHelpers::User1.emails.first }
+
+          let!(:user) { create(:user, email: GpgHelpers::User1.emails.first) }
+
+          let!(:gpg_key) do
+            create :gpg_key, key: GpgHelpers::User1.public_key, user: user
+          end
+
+          let!(:crypto) { instance_double(GPGME::Crypto) }
+
+          before do
+            fake_signature = [
+              GpgHelpers::User1.signed_commit_signature,
+              GpgHelpers::User1.signed_commit_base_data
+            ]
+
+            allow(Gitlab::Git::Commit).to receive(:extract_signature_lazily)
+              .with(Gitlab::Git::Repository, commit_sha)
+              .and_return(fake_signature)
+          end
+
+          it 'returns an invalid signatures error' do
+            verified_signature = double('verified-signature', fingerprint: GpgHelpers::User1.fingerprint, valid?: true)
+            allow(GPGME::Crypto).to receive(:new).and_return(crypto)
+            allow(crypto).to receive(:verify).and_yield(verified_signature).and_yield(verified_signature)
+
+            signature = described_class.new(commit).signature
+
+            expect(signature).to have_attributes(
+              commit_sha: commit_sha,
+              project: project,
+              gpg_key: gpg_key,
+              gpg_key_primary_keyid: GpgHelpers::User1.primary_keyid,
+              gpg_key_user_name: GpgHelpers::User1.names.first,
+              gpg_key_user_email: GpgHelpers::User1.emails.first,
+              verification_status: 'multiple_signatures'
+            )
+          end
+
+          context 'when feature flag is disabled' do
+            before do
+              stub_feature_flags(multiple_gpg_signatures: false)
+            end
+
+            it 'returns an valid signature' do
+              verified_signature = double('verified-signature', fingerprint: GpgHelpers::User1.fingerprint, valid?: true)
+              allow(GPGME::Crypto).to receive(:new).and_return(crypto)
+              allow(crypto).to receive(:verify).and_yield(verified_signature).and_yield(verified_signature)
+
+              signature = described_class.new(commit).signature
+
+              expect(signature).to have_attributes(
+                commit_sha: commit_sha,
+                project: project,
+                gpg_key: gpg_key,
+                gpg_key_primary_keyid: GpgHelpers::User1.primary_keyid,
+                gpg_key_user_name: GpgHelpers::User1.names.first,
+                gpg_key_user_email: GpgHelpers::User1.emails.first,
+                verification_status: 'verified'
+              )
+            end
+          end
+        end
+
         context 'commit signed with a subkey' do
           let!(:commit) { create :commit, project: project, sha: commit_sha, committer_email: GpgHelpers::User3.emails.first }
 

spec/support/helpers/gpg_helpers.rb

@@ -4,6 +4,7 @@ module GpgHelpers
   SIGNED_COMMIT_SHA       = '8a852d50dda17cc8fd1408d2fd0c5b0f24c76ca4'
   SIGNED_AND_AUTHORED_SHA = '3c1d9a0266cb0c62d926f4a6c649beed561846f5'
   DIFFERING_EMAIL_SHA     = 'a17a9f66543673edf0a3d1c6b93bdda3fe600f32'
+  MULTIPLE_SIGNATURES_SHA = 'c7794c14268d67ad8a2d5f066d706539afc75a96'
 
   module User1
     extend self

spec/support/helpers/test_env.rb

@@ -9,7 +9,7 @@ module TestEnv
 
   # When developing the seed repository, comment out the branch you will modify.
   BRANCH_SHA = {
-    'signed-commits'                     => '6101e87',
+    'signed-commits'                     => 'c7794c1',
     'not-merged-branch'                  => 'b83d6e3',
     'branch-merged'                      => '498214d',
     'empty-branch'                       => '7efb185',