Merge branch 'fix/tag-signature-verification' into 'master'

Fix verification of signed tags with long messages

See merge request gitlab-org/gitlab!67000

config/feature_flags/development/get_tag_signatures.yml

+---
+name: get_tag_signatures
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/67000
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/337842
+milestone: '14.2'
+type: development
+group: group::gitaly
+default_enabled: false

lib/gitlab/git/tag.rb

@@ -5,6 +5,8 @@ module Gitlab
     class Tag < Ref
       extend Gitlab::EncodingHelper
 
+      delegate :id, to: :@raw_tag
+
       attr_reader :object_sha, :repository
 
       MAX_TAG_MESSAGE_DISPLAY_SIZE = 10.megabytes
@@ -24,6 +26,18 @@ module Gitlab
         def get_messages(repository, tag_ids)
           repository.gitaly_ref_client.get_tag_messages(tag_ids)
         end
+
+        def extract_signature_lazily(repository, tag_id)
+          BatchLoader.for(tag_id).batch(key: repository) do |tag_ids, loader, args|
+            batch_signature_extraction(args[:key], tag_ids).each do |tag_id, signature_data|
+              loader.call(tag_id, signature_data)
+            end
+          end
+        end
+
+        def batch_signature_extraction(repository, tag_ids)
+          repository.gitaly_ref_client.get_tag_signatures(tag_ids)
+        end
       end
 
       def initialize(repository, raw_tag)
@@ -81,7 +95,7 @@ module Gitlab
         when :PGP
           nil # not implemented, see https://gitlab.com/gitlab-org/gitlab/issues/19260
         when :X509
-          X509::Tag.new(@raw_tag).signature
+          X509::Tag.new(@repository, self).signature
         else
           nil
         end

lib/gitlab/gitaly_client/ref_service.rb

@@ -178,6 +178,27 @@ module Gitlab
         messages
       end
 
+      def get_tag_signatures(tag_ids)
+        request = Gitaly::GetTagSignaturesRequest.new(repository: @gitaly_repo, tag_revisions: tag_ids)
+        response = GitalyClient.call(@repository.storage, :ref_service, :get_tag_signatures, request, timeout: GitalyClient.fast_timeout)
+
+        signatures = Hash.new { |h, k| h[k] = [+''.b, +''.b] }
+        current_tag_id = nil
+
+        response.each do |message|
+          message.signatures.each do |tag_signature|
+            current_tag_id = tag_signature.tag_id if tag_signature.tag_id.present?
+
+            signatures[current_tag_id].first << tag_signature.signature
+            signatures[current_tag_id].last << tag_signature.content
+          end
+        end
+
+        signatures
+      rescue GRPC::InvalidArgument => ex
+        raise ArgumentError, ex
+      end
+
       def pack_refs
         request = Gitaly::PackRefsRequest.new(repository: @gitaly_repo)
 

lib/gitlab/signed_tag.rb

+# frozen_string_literal: true
+
+module Gitlab
+  class SignedTag
+    include Gitlab::Utils::StrongMemoize
+
+    def initialize(repository, tag)
+      @repository = repository
+      @tag = tag
+
+      if Feature.enabled?(:get_tag_signatures)
+        @signature_data = Gitlab::Git::Tag.extract_signature_lazily(repository, tag.id) if repository
+      else
+        @signature_data = [signature_text_of_message.b, signed_text_of_message.b]
+      end
+    end
+
+    def signature
+      return unless @tag.has_signature?
+    end
+
+    def signature_text
+      @signature_data&.fetch(0)
+    end
+
+    def signed_text
+      @signature_data&.fetch(1)
+    end
+
+    private
+
+    def signature_text_of_message
+      @tag.message.slice(@tag.message.index("-----BEGIN SIGNED MESSAGE-----")..-1)
+    rescue StandardError
+      nil
+    end
+
+    def signed_text_of_message
+      %{object #{@tag.target_commit.id}
+type commit
+tag #{@tag.name}
+tagger #{@tag.tagger.name} <#{@tag.tagger.email}> #{@tag.tagger.date.seconds} #{@tag.tagger.timezone}
+
+#{@tag.message.gsub(/-----BEGIN SIGNED MESSAGE-----(.*)-----END SIGNED MESSAGE-----/m, "")}}
+    end
+  end
+end

lib/gitlab/x509/tag.rb

@@ -4,37 +4,16 @@ require 'digest'
 
 module Gitlab
   module X509
-    class Tag
+    class Tag < Gitlab::SignedTag
       include Gitlab::Utils::StrongMemoize
 
-      def initialize(raw_tag)
-        @raw_tag = raw_tag
-      end
-
       def signature
-        signature = X509::Signature.new(signature_text, signed_text, @raw_tag.tagger.email, Time.at(@raw_tag.tagger.date.seconds))
-
-        return if signature.verified_signature.nil?
-
-        signature
-      end
-
-      private
-
-      def signature_text
-        @raw_tag.message.slice(@raw_tag.message.index("-----BEGIN SIGNED MESSAGE-----")..-1)
-      rescue StandardError
-        nil
-      end
-
-      def signed_text
-        # signed text is reconstructed as long as there is no specific gitaly function
-        %{object #{@raw_tag.target_commit.id}
-type commit
-tag #{@raw_tag.name}
-tagger #{@raw_tag.tagger.name} <#{@raw_tag.tagger.email}> #{@raw_tag.tagger.date.seconds} #{@raw_tag.tagger.timezone}
+        strong_memoize(:signature) do
+          super
 
-#{@raw_tag.message.gsub(/-----BEGIN SIGNED MESSAGE-----(.*)-----END SIGNED MESSAGE-----/m, "")}}
+          signature = X509::Signature.new(signature_text, signed_text, @tag.tagger.email, Time.at(@tag.tagger.date.seconds))
+          signature unless signature.verified_signature.nil?
+        end
       end
     end
   end

spec/lib/gitlab/git/tag_spec.rb

@@ -38,7 +38,7 @@ RSpec.describe Gitlab::Git::Tag, :seed_helper do
       it { expect(tag.tagger.timezone).to eq("+0200") }
     end
 
-    describe 'signed tag' do
+    shared_examples 'signed tag' do
       let(:project) { create(:project, :repository) }
       let(:tag) { project.repository.find_tag('v1.1.1') }
 
@@ -54,6 +54,18 @@ RSpec.describe Gitlab::Git::Tag, :seed_helper do
       it { expect(tag.tagger.timezone).to eq("+0100") }
     end
 
+    context 'with :get_tag_signatures enabled' do
+      it_behaves_like 'signed tag'
+    end
+
+    context 'with :get_tag_signatures disabled' do
+      before do
+        stub_feature_flags(get_tag_signatures: false)
+      end
+
+      it_behaves_like 'signed tag'
+    end
+
     it { expect(repository.tags.size).to eq(SeedRepo::Repo::TAGS.size) }
   end
 
@@ -77,6 +89,75 @@ RSpec.describe Gitlab::Git::Tag, :seed_helper do
     end
   end
 
+  describe '.extract_signature_lazily' do
+    let(:project) { create(:project, :repository) }
+
+    subject { described_class.extract_signature_lazily(project.repository, tag_id).itself }
+
+    context 'when the tag is signed' do
+      let(:tag_id) { project.repository.find_tag('v1.1.1').id }
+
+      it 'returns signature and signed text' do
+        signature, signed_text = subject
+
+        expect(signature).to eq(X509Helpers::User1.signed_tag_signature.chomp)
+        expect(signature).to be_a_binary_string
+        expect(signed_text).to eq(X509Helpers::User1.signed_tag_base_data)
+        expect(signed_text).to be_a_binary_string
+      end
+    end
+
+    context 'when the tag has no signature' do
+      let(:tag_id) { project.repository.find_tag('v1.0.0').id }
+
+      it 'returns empty signature and message as signed text' do
+        signature, signed_text = subject
+
+        expect(signature).to be_empty
+        expect(signed_text).to eq(X509Helpers::User1.unsigned_tag_base_data)
+        expect(signed_text).to be_a_binary_string
+      end
+    end
+
+    context 'when the tag cannot be found' do
+      let(:tag_id) { Gitlab::Git::BLANK_SHA }
+
+      it 'raises GRPC::Internal' do
+        expect { subject }.to raise_error(GRPC::Internal)
+      end
+    end
+
+    context 'when the tag ID is invalid' do
+      let(:tag_id) { '4b4918a572fa86f9771e5ba40fbd48e' }
+
+      it 'raises GRPC::Internal' do
+        expect { subject }.to raise_error(GRPC::Internal)
+      end
+    end
+
+    context 'when loading signatures in batch once' do
+      it 'fetches signatures in batch once' do
+        tag_ids = [project.repository.find_tag('v1.1.1').id, project.repository.find_tag('v1.0.0').id]
+        signatures = tag_ids.map do |tag_id|
+          described_class.extract_signature_lazily(repository, tag_id)
+        end
+
+        other_repository = double(:repository)
+        described_class.extract_signature_lazily(other_repository, tag_ids.first)
+
+        expect(described_class).to receive(:batch_signature_extraction)
+          .with(repository, tag_ids)
+          .once
+          .and_return({})
+
+        expect(described_class).not_to receive(:batch_signature_extraction)
+          .with(other_repository, tag_ids.first)
+
+        2.times { signatures.each(&:itself) }
+      end
+    end
+  end
+
   describe 'tag into from Gitaly tag' do
     context 'message_size != message.size' do
       let(:gitaly_tag) { build(:gitaly_tag, message: ''.b, message_size: message_size) }

spec/lib/gitlab/gitaly_client/ref_service_spec.rb

@@ -178,6 +178,17 @@ RSpec.describe Gitlab::GitalyClient::RefService do
     end
   end
 
+  describe '#get_tag_signatures' do
+    it 'sends a get_tag_signatures message' do
+      expect_any_instance_of(Gitaly::RefService::Stub)
+        .to receive(:get_tag_signatures)
+        .with(gitaly_request_with_params(tag_revisions: ['some_tag_id']), kind_of(Hash))
+        .and_return([])
+
+      client.get_tag_signatures(['some_tag_id'])
+    end
+  end
+
   describe '#find_ref_name', :seed_helper do
     subject { client.find_ref_name(SeedRepo::Commit::ID, 'refs/heads/master') }
 

spec/lib/gitlab/x509/tag_spec.rb

@@ -2,13 +2,13 @@
 require 'spec_helper'
 
 RSpec.describe Gitlab::X509::Tag do
-  subject(:signature) { described_class.new(tag).signature }
+  subject(:signature) { described_class.new(project.repository, tag).signature }
 
   describe '#signature' do
     let(:repository) { Gitlab::Git::Repository.new('default', TEST_REPO_PATH, '', 'group/project') }
     let(:project) { create(:project, :repository) }
 
-    describe 'signed tag' do
+    shared_examples 'signed tag' do
       let(:tag) { project.repository.find_tag('v1.1.1') }
       let(:certificate_attributes) do
         {
@@ -33,10 +33,24 @@ RSpec.describe Gitlab::X509::Tag do
       it { expect(signature.x509_certificate.x509_issuer).to have_attributes(issuer_attributes) }
     end
 
-    context 'unsigned tag' do
+    shared_examples 'unsigned tag' do
       let(:tag) { project.repository.find_tag('v1.0.0') }
 
       it { expect(signature).to be_nil }
     end
+
+    context 'with :get_tag_signatures enabled' do
+      it_behaves_like 'signed tag'
+      it_behaves_like 'unsigned tag'
+    end
+
+    context 'with :get_tag_signatures disabled' do
+      before do
+        stub_feature_flags(get_tag_signatures: false)
+      end
+
+      it_behaves_like 'signed tag'
+      it_behaves_like 'unsigned tag'
+    end
   end
 end

spec/support/helpers/x509_helpers.rb

@@ -290,6 +290,17 @@ module X509Helpers
       SIGNEDDATA
     end
 
+    def unsigned_tag_base_data
+      <<~SIGNEDDATA
+      object 6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9
+      type commit
+      tag v1.0.0
+      tagger Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com> 1393491299 +0200
+      
+      Release
+      SIGNEDDATA
+    end
+
     def certificate_crl
       'http://ch.siemens.com/pki?ZZZZZZA2.crl'
     end