Only verify commit signatures if the user email is verified

Currently, users are able to create "verified" commit signatures for
emails they don't control.

Changelog: security

lib/gitlab/x509/signature.rb

@@ -23,7 +23,7 @@ module Gitlab
       end
 
       def user
-        User.find_by_any_email(@email)
+        strong_memoize(:user) { User.find_by_any_email(@email) }
       end
 
       def verified_signature
@@ -31,9 +31,13 @@ module Gitlab
       end
 
       def verification_status
-        return :unverified if x509_certificate.nil? || x509_certificate.revoked?
+        return :unverified if
+          x509_certificate.nil? ||
+          x509_certificate.revoked? ||
+          !verified_signature ||
+          user.nil?
 
-        if verified_signature && certificate_email == @email
+        if user.verified_emails.include?(@email) && certificate_email == @email
           :verified
         else
           :unverified

spec/lib/gitlab/x509/signature_spec.rb

@@ -12,20 +12,30 @@ RSpec.describe Gitlab::X509::Signature do
   end
 
   shared_examples "a verified signature" do
-    it 'returns a verified signature if email does match' do
-      signature = described_class.new(
+    let_it_be(:user) { create(:user, email: X509Helpers::User1.certificate_email) }
+
+    subject(:signature) do
+      described_class.new(
         X509Helpers::User1.signed_commit_signature,
         X509Helpers::User1.signed_commit_base_data,
         X509Helpers::User1.certificate_email,
         X509Helpers::User1.signed_commit_time
       )
+    end
 
+    it 'returns a verified signature if email does match' do
       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 the email matches but isn't confirmed" do
+      user.update!(confirmed_at: nil)
+
+      expect(signature.verification_status).to eq(:unverified)
+    end
+
     it 'returns an unverified signature if email does not match' do
       signature = described_class.new(
         X509Helpers::User1.signed_commit_signature,
@@ -55,13 +65,6 @@ RSpec.describe Gitlab::X509::Signature do
     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!
@@ -253,23 +256,25 @@ RSpec.describe Gitlab::X509::Signature do
   end
 
   describe '#user' do
-    signature = described_class.new(
-      X509Helpers::User1.signed_tag_signature,
-      X509Helpers::User1.signed_tag_base_data,
-      X509Helpers::User1.certificate_email,
-      X509Helpers::User1.signed_commit_time
-    )
+    subject do
+      described_class.new(
+        X509Helpers::User1.signed_tag_signature,
+        X509Helpers::User1.signed_tag_base_data,
+        X509Helpers::User1.certificate_email,
+        X509Helpers::User1.signed_commit_time
+      ).user
+    end
 
     context 'if email is assigned to a user' do
       let!(:user) { create(:user, email: X509Helpers::User1.certificate_email) }
 
       it 'returns user' do
-        expect(signature.user).to eq(user)
+        is_expected.to eq(user)
       end
     end
 
     it 'if email is not assigned to a user, return nil' do
-      expect(signature.user).to be_nil
+      is_expected.to be_nil
     end
   end
 
@@ -292,6 +297,17 @@ RSpec.describe Gitlab::X509::Signature do
     end
 
     context 'verified signature' do
+      let_it_be(:user) { create(:user, email: X509Helpers::User1.certificate_email) }
+
+      subject(:signature) do
+        described_class.new(
+          X509Helpers::User1.signed_tag_signature,
+          X509Helpers::User1.signed_tag_base_data,
+          X509Helpers::User1.certificate_email,
+          X509Helpers::User1.signed_commit_time
+        )
+      end
+
       context 'with trusted certificate store' do
         before do
           store = OpenSSL::X509::Store.new
@@ -301,19 +317,18 @@ RSpec.describe Gitlab::X509::Signature do
         end
 
         it 'returns a verified signature if email does match' do
-          signature = described_class.new(
-            X509Helpers::User1.signed_tag_signature,
-            X509Helpers::User1.signed_tag_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 the email matches but isn't confirmed" do
+          user.update!(confirmed_at: nil)
+
+          expect(signature.verification_status).to eq(:unverified)
+        end
+
         it 'returns an unverified signature if email does not match' do
           signature = described_class.new(
             X509Helpers::User1.signed_tag_signature,
@@ -343,13 +358,6 @@ RSpec.describe Gitlab::X509::Signature do
         end
 
         it 'returns an unverified signature if certificate is revoked' do
-          signature = described_class.new(
-            X509Helpers::User1.signed_tag_signature,
-            X509Helpers::User1.signed_tag_base_data,
-            X509Helpers::User1.certificate_email,
-            X509Helpers::User1.signed_commit_time
-          )
-
           expect(signature.verification_status).to eq(:verified)
 
           signature.x509_certificate.revoked!
@@ -368,13 +376,6 @@ RSpec.describe Gitlab::X509::Signature do
         end
 
         it 'returns an unverified signature' do
-          signature = described_class.new(
-            X509Helpers::User1.signed_tag_signature,
-            X509Helpers::User1.signed_tag_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

spec/tasks/gitlab/x509/update_rake_spec.rb

@@ -8,12 +8,13 @@ RSpec.describe 'gitlab:x509 namespace rake task' do
   end
 
   describe 'update_signatures' do
-    subject { run_rake_task('gitlab:x509:update_signatures') }
-
-    let(:project) { create :project, :repository, path: X509Helpers::User1.path }
+    let(:user) { create(:user, email: X509Helpers::User1.certificate_email) }
+    let(:project) { create(:project, :repository, path: X509Helpers::User1.path, creator: user) }
     let(:x509_signed_commit) { project.commit_by(oid: '189a6c924013fc3fe40d6f1ec1dc20214183bc97') }
     let(:x509_commit) { Gitlab::X509::Commit.new(x509_signed_commit).signature }
 
+    subject { run_rake_task('gitlab:x509:update_signatures') }
+
     it 'changes from unverified to verified if the certificate store contains the root certificate' do
       x509_commit
 
@@ -22,21 +23,14 @@ RSpec.describe 'gitlab:x509 namespace rake task' do
       store.add_cert(certificate)
       allow(OpenSSL::X509::Store).to receive(:new).and_return(store)
 
-      expect(x509_commit.verification_status).to eq('unverified')
       expect_any_instance_of(Gitlab::X509::Commit).to receive(:update_signature!).and_call_original
-
-      subject
-
-      x509_commit.reload
-      expect(x509_commit.verification_status).to eq('verified')
+      expect { subject }.to change { x509_commit.reload.verification_status }.from('unverified').to('verified')
     end
 
     it 'returns if no signature is available' do
-      expect_any_instance_of(Gitlab::X509::Commit) do |x509_commit|
-        expect(x509_commit).not_to receive(:update_signature!)
+      expect_any_instance_of(Gitlab::X509::Commit).not_to receive(:update_signature!)
 
-        subject
-      end
+      subject
     end
   end
 end