Merge branch 'sh-handle-invalid-gpg-sig' into 'master'

Gracefully handle unknown/invalid GPG keys

Closes #54729

See merge request gitlab-org/gitlab-ce!23492

3 files changed, 44 insertions, 7 deletions

diff --git a/changelogs/unreleased/sh-handle-invalid-gpg-sig.yml b/changelogs/unreleased/sh-handle-invalid-gpg-sig.yml
new file mode 100644
index 00000000000..185e2547e16
--- /dev/null
+++ b/changelogs/unreleased/sh-handle-invalid-gpg-sig.yml
@@ -0,0 +1,5 @@
+---
+title: Gracefully handle unknown/invalid GPG keys
+merge_request: 23492
+author:
+type: fixed
diff --git a/lib/gitlab/gpg/commit.rb b/lib/gitlab/gpg/commit.rb
index 31bab20b044..4fbb87385c3 100644
--- a/lib/gitlab/gpg/commit.rb
+++ b/lib/gitlab/gpg/commit.rb
@@ -44,9 +44,8 @@ module Gitlab
       def update_signature!(cached_signature)
         using_keychain do |gpg_key|
           cached_signature.update!(attributes(gpg_key))
+          @signature = cached_signature
         end
-
-        @signature = cached_signature
       end
 
       private
@@ -59,11 +58,15 @@ module Gitlab
           # the proper signature.
           # NOTE: the invoked method is #fingerprint but it's only returning
           # 16 characters (the format used by keyid) instead of 40.
-          gpg_key = find_gpg_key(verified_signature.fingerprint)
+          fingerprint = verified_signature&.fingerprint
+
+          break unless fingerprint
+
+          gpg_key = find_gpg_key(fingerprint)
 
           if gpg_key
             Gitlab::Gpg::CurrentKeyChain.add(gpg_key.key)
-            @verified_signature = nil
+            clear_memoization(:verified_signature)
           end
 
           yield gpg_key
@@ -71,9 +74,16 @@ module Gitlab
       end
 
       def verified_signature
-        @verified_signature ||= GPGME::Crypto.new.verify(signature_text, signed_text: signed_text) do |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-ce/issues/54932
           break verified_signature
         end
+      rescue GPGME::Error
+        nil
       end
 
       def create_cached_signature!
@@ -92,7 +102,7 @@ module Gitlab
           commit_sha: @commit.sha,
           project: @commit.project,
           gpg_key: gpg_key,
-          gpg_key_primary_keyid: gpg_key&.keyid || verified_signature.fingerprint,
+          gpg_key_primary_keyid: gpg_key&.keyid || verified_signature&.fingerprint,
           gpg_key_user_name: user_infos[:name],
           gpg_key_user_email: user_infos[:email],
           verification_status: verification_status
@@ -102,7 +112,7 @@ module Gitlab
       def verification_status(gpg_key)
         return :unknown_key unless gpg_key
         return :unverified_key unless gpg_key.verified?
-        return :unverified unless verified_signature.valid?
+        return :unverified unless verified_signature&.valid?
 
         if gpg_key.verified_and_belongs_to_email?(@commit.committer_email)
           :verified
diff --git a/spec/lib/gitlab/gpg/commit_spec.rb b/spec/lib/gitlab/gpg/commit_spec.rb
index 8c6d673391b..8229f0eb794 100644
--- a/spec/lib/gitlab/gpg/commit_spec.rb
+++ b/spec/lib/gitlab/gpg/commit_spec.rb
@@ -26,6 +26,28 @@ describe Gitlab::Gpg::Commit do
       end
     end
 
+    context 'invalid signature' 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) }
+
+      before do
+        allow(Gitlab::Git::Commit).to receive(:extract_signature_lazily)
+            .with(Gitlab::Git::Repository, commit_sha)
+            .and_return(
+              [
+                # Corrupt the key
+                GpgHelpers::User1.signed_commit_signature.tr('=', 'a'),
+                GpgHelpers::User1.signed_commit_base_data
+              ]
+            )
+      end
+
+      it 'returns nil' do
+        expect(described_class.new(commit).signature).to be_nil
+      end
+    end
+
     context 'known key' do
       context 'user matches the key uid' do
         context 'user email matches the email committer' do