Merge branch 'feat/x509-crl' into 'master'

Add functionality to revoke a X509Certificate and update related X509CommitSignatures

See merge request gitlab-org/gitlab!24889

app/models/x509_certificate.rb

@@ -2,6 +2,7 @@
 
 class X509Certificate < ApplicationRecord
   include X509SerialNumberAttribute
+  include AfterCommitQueue
 
   x509_serial_number_attribute :serial_number
 
@@ -25,8 +26,14 @@ class X509Certificate < ApplicationRecord
 
   validates :x509_issuer_id, presence: true
 
+  after_commit :mark_commit_signatures_unverified
+
   def self.safe_create!(attributes)
     create_with(attributes)
       .safe_find_or_create_by!(subject_key_identifier: attributes[:subject_key_identifier])
   end
+
+  def mark_commit_signatures_unverified
+    X509CertificateRevokeWorker.perform_async(self.id) if revoked?
+  end
 end

app/services/x509_certificate_revoke_service.rb

+# frozen_string_literal: true
+
+class X509CertificateRevokeService
+  def execute(certificate)
+    return unless certificate.revoked?
+
+    certificate.x509_commit_signatures.update_all(verification_status: :unverified)
+  end
+end

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

 .gpg-popover-certificate-details
   %strong= _('Certificate Subject')
+  - if signature.x509_certificate.revoked?
+    %strong.cred= _('(revoked)')
   %ul
     - x509_subject(signature.x509_certificate.subject, ["CN", "O"]).map do |key, value|
       %li= key + "=" + value

app/workers/all_queues.yml

@@ -1291,3 +1291,10 @@
   :resource_boundary: :unknown
   :weight: 1
   :idempotent: 
+- :name: x509_certificate_revoke
+  :feature_category: :source_code_management
+  :has_external_dependencies: 
+  :urgency: :default
+  :resource_boundary: :unknown
+  :weight: 1
+  :idempotent: true

app/workers/x509_certificate_revoke_worker.rb

+# frozen_string_literal: true
+
+class X509CertificateRevokeWorker
+  include ApplicationWorker
+
+  feature_category :source_code_management
+
+  idempotent!
+
+  def perform(certificate_id)
+    return unless certificate_id
+
+    X509Certificate.find_by_id(certificate_id).try do |certificate|
+      X509CertificateRevokeService.new.execute(certificate)
+    end
+  end
+end

changelogs/unreleased/feat-x509-crl.yml

+---
+title: Add functionality to revoke a X509Certificate and update related X509CommitSignatures
+merge_request: 24889
+author: Roger Meier
+type: added

config/sidekiq_queues.yml

@@ -248,3 +248,5 @@
   - 1
 - - web_hook
   - 1
+- - x509_certificate_revoke
+  - 1

lib/gitlab/x509/commit.rb

@@ -184,11 +184,13 @@ module Gitlab
           commit_sha: @commit.sha,
           project: @commit.project,
           x509_certificate_id: certificate.id,
-          verification_status: verification_status
+          verification_status: verification_status(certificate)
         }
       end
 
-      def verification_status
+      def verification_status(certificate)
+        return :unverified if certificate.revoked?
+
         if verified_signature && certificate_email == @commit.committer_email
           :verified
         else

locale/gitlab.pot

@@ -560,6 +560,9 @@ msgstr ""
 msgid "(removed)"
 msgstr ""
 
+msgid "(revoked)"
+msgstr ""
+
 msgid "*"
 msgstr ""
 

spec/factories/x509_certificate.rb

@@ -8,5 +8,6 @@ FactoryBot.define do
     email { 'gitlab@example.org' }
     serial_number { 278969561018901340486471282831158785578 }
     x509_issuer
+    certificate_status { :good }
   end
 end

spec/lib/gitlab/x509/commit_spec.rb

@@ -111,6 +111,22 @@ describe Gitlab::X509::Commit do
           expect(signature.x509_certificate.x509_issuer).to have_attributes(user1_issuer_attributes)
           expect(signature.persisted?).to be_truthy
         end
+
+        context 'revoked certificate' do
+          let(:x509_issuer) { create(:x509_issuer, user1_issuer_attributes) }
+          let!(:x509_certificate) { create(:x509_certificate, user1_certificate_attributes.merge(x509_issuer_id: x509_issuer.id, certificate_status: :revoked)) }
+
+          it 'returns an unverified signature' do
+            expect(signature).to have_attributes(
+              commit_sha: commit_sha,
+              project: project,
+              verification_status: 'unverified'
+            )
+            expect(signature.x509_certificate).to have_attributes(user1_certificate_attributes)
+            expect(signature.x509_certificate.x509_issuer).to have_attributes(user1_issuer_attributes)
+            expect(signature.persisted?).to be_truthy
+          end
+        end
       end
 
       context 'without trusted certificate within store' do

spec/models/x509_certificate_spec.rb

@@ -43,6 +43,28 @@ RSpec.describe X509Certificate do
       expect(certificate.subject).to eq(subject)
       expect(certificate.email).to eq(email)
     end
+
+    it 'calls mark_commit_signatures_unverified' do
+      expect_any_instance_of(described_class).to receive(:mark_commit_signatures_unverified)
+
+      described_class.safe_create!(attributes)
+    end
+
+    context 'certificate revocation handling' do
+      let(:x509_certificate) { create(:x509_certificate) }
+
+      it 'starts a revoke worker if certificate is revoked' do
+        expect(X509CertificateRevokeWorker).to receive(:perform_async).with(x509_certificate.id)
+
+        x509_certificate.revoked!
+      end
+
+      it 'does not starts a revoke worker for good certificates' do
+        expect(X509CertificateRevokeWorker).not_to receive(:perform_async).with(x509_certificate.id)
+
+        x509_certificate
+      end
+    end
   end
 
   describe 'validators' do

spec/services/x509_certificate_revoke_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe X509CertificateRevokeService do
+  describe '#execute' do
+    let(:service) { described_class.new }
+    let!(:x509_signature_1) { create(:x509_commit_signature, x509_certificate: x509_certificate, verification_status: :verified ) }
+    let!(:x509_signature_2) { create(:x509_commit_signature, x509_certificate: x509_certificate, verification_status: :verified ) }
+
+    context 'for revoked certificates' do
+      let(:x509_certificate) { create(:x509_certificate, certificate_status: :revoked ) }
+
+      it 'update all commit signatures' do
+        expect do
+          service.execute(x509_certificate)
+
+          x509_signature_1.reload
+          x509_signature_2.reload
+        end
+          .to change(x509_signature_1, :verification_status).from('verified').to('unverified')
+          .and change(x509_signature_2, :verification_status).from('verified').to('unverified')
+      end
+    end
+
+    context 'for good certificates' do
+      RSpec::Matchers.define_negated_matcher :not_change, :change
+
+      let(:x509_certificate) { create(:x509_certificate) }
+
+      it 'do not update any commit signature' do
+        expect do
+          service.execute(x509_certificate)
+
+          x509_signature_1.reload
+          x509_signature_2.reload
+        end
+          .to not_change(x509_signature_1, :verification_status)
+          .and not_change(x509_signature_2, :verification_status)
+      end
+    end
+  end
+end

spec/workers/x509_certificate_revoke_worker_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe X509CertificateRevokeWorker do
+  describe '#perform' do
+    context 'with a revoked certificate' do
+      subject { described_class.new }
+
+      let(:x509_certificate) { create(:x509_certificate, certificate_status: :revoked) }
+      let(:job_args) { x509_certificate.id }
+
+      include_examples 'an idempotent worker' do
+        it 'executes the revoke service' do
+          spy_service = X509CertificateRevokeService.new
+
+          allow(X509CertificateRevokeService).to receive(:new) { spy_service }
+
+          expect(spy_service).to receive(:execute)
+            .exactly(IdempotentWorkerHelper::WORKER_EXEC_TIMES).times
+            .with(x509_certificate)
+            .and_call_original
+
+          subject
+        end
+      end
+
+      it 'executes the revoke service' do
+        spy_service = X509CertificateRevokeService.new
+
+        allow(X509CertificateRevokeService).to receive(:new) { spy_service }
+
+        expect_next_instance_of(X509CertificateRevokeService) do |service|
+          expect(service).to receive(:execute).with(x509_certificate)
+        end
+
+        subject
+      end
+    end
+  end
+end