Read about our upcoming Code of Conduct on this issue

Commit dd42cb5f authored by Douwe Maan's avatar Douwe Maan
Browse files

Merge branch '36829-gpg-commit-not-verified-if-signed-with-a-subkey' into 'master'

Add support for GPG subkeys in signature verification

Closes #36829

See merge request gitlab-org/gitlab-ce!14517
......@@ -108,6 +108,15 @@
}
}
.subkeys-list {
@include basic-list;
li {
padding: 3px 0;
border: none;
}
}
.key-list-item {
.key-list-item-info {
@media (min-width: $screen-sm-min) {
......
......@@ -2,7 +2,7 @@ class Profiles::GpgKeysController < Profiles::ApplicationController
before_action :set_gpg_key, only: [:destroy, :revoke]
def index
@gpg_keys = current_user.gpg_keys
@gpg_keys = current_user.gpg_keys.with_subkeys
@gpg_key = GpgKey.new
end
......
......@@ -9,6 +9,9 @@ class GpgKey < ActiveRecord::Base
belongs_to :user
has_many :gpg_signatures
has_many :subkeys, class_name: 'GpgKeySubkey'
scope :with_subkeys, -> { includes(:subkeys) }
validates :user, presence: true
......@@ -36,10 +39,12 @@ class GpgKey < ActiveRecord::Base
before_validation :extract_fingerprint, :extract_primary_keyid
after_commit :update_invalid_gpg_signatures, on: :create
after_create :generate_subkeys
def primary_keyid
super&.upcase
end
alias_method :keyid, :primary_keyid
def fingerprint
super&.upcase
......@@ -49,6 +54,10 @@ def key=(value)
super(value&.strip)
end
def keyids
[keyid].concat(subkeys.map(&:keyid))
end
def user_infos
@user_infos ||= Gitlab::Gpg.user_infos_from_key(key)
end
......@@ -82,10 +91,11 @@ def update_invalid_gpg_signatures
def revoke
GpgSignature
.where(gpg_key: self)
.with_key_and_subkeys(self)
.where.not(verification_status: GpgSignature.verification_statuses[:unknown_key])
.update_all(
gpg_key_id: nil,
gpg_key_subkey_id: nil,
verification_status: GpgSignature.verification_statuses[:unknown_key],
updated_at: Time.zone.now
)
......@@ -106,4 +116,12 @@ def extract_primary_keyid
# only allows one key
self.primary_keyid = Gitlab::Gpg.primary_keyids_from_key(key).first
end
def generate_subkeys
gpg_subkeys = Gitlab::Gpg.subkeys_from_key(key)
gpg_subkeys[primary_keyid]&.each do |subkey_data|
subkeys.create!(keyid: subkey_data[:keyid], fingerprint: subkey_data[:fingerprint])
end
end
end
class GpgKeySubkey < ActiveRecord::Base
include ShaAttribute
sha_attribute :keyid
sha_attribute :fingerprint
belongs_to :gpg_key
validates :gpg_key_id, presence: true
validates :fingerprint, :keyid, presence: true, uniqueness: true
delegate :key, :user, :user_infos, :verified?, :verified_user_infos,
:verified_and_belongs_to_email?, to: :gpg_key
def keyid
super&.upcase
end
def fingerprint
super&.upcase
end
end
......@@ -15,11 +15,42 @@ class GpgSignature < ActiveRecord::Base
belongs_to :project
belongs_to :gpg_key
belongs_to :gpg_key_subkey
validates :commit_sha, presence: true
validates :project_id, presence: true
validates :gpg_key_primary_keyid, presence: true
def self.with_key_and_subkeys(gpg_key)
subkey_ids = gpg_key.subkeys.pluck(:id)
where(
arel_table[:gpg_key_id].eq(gpg_key.id).or(
arel_table[:gpg_key_subkey_id].in(subkey_ids)
)
)
end
def gpg_key=(model)
case model
when GpgKey
super
when GpgKeySubkey
self.gpg_key_subkey = model
when NilClass
super
self.gpg_key_subkey = nil
end
end
def gpg_key
if gpg_key_id
super
elsif gpg_key_subkey_id
gpg_key_subkey
end
end
def gpg_key_primary_keyid
super&.upcase
end
......
......@@ -7,6 +7,13 @@
.description
%code= key.fingerprint
- if key.subkeys.present?
.subkeys
%span.bold Subkeys:
%ul.subkeys-list
- key.subkeys.each do |subkey|
%li
%code= subkey.fingerprint
.pull-right
%span.key-created-at
created #{time_ago_with_tooltip(key.created_at)}
......
---
title: Add support for GPG subkeys in signature verification
merge_request: 14517
author:
type: added
class CreateGpgKeySubkeys < ActiveRecord::Migration
DOWNTIME = false
def up
create_table :gpg_key_subkeys do |t|
t.references :gpg_key, null: false, index: true, foreign_key: { on_delete: :cascade }
t.binary :keyid
t.binary :fingerprint
t.index :keyid, unique: true, length: Gitlab::Database.mysql? ? 20 : nil
t.index :fingerprint, unique: true, length: Gitlab::Database.mysql? ? 20 : nil
end
add_reference :gpg_signatures, :gpg_key_subkey, index: true, foreign_key: { on_delete: :nullify }
end
def down
remove_reference(:gpg_signatures, :gpg_key_subkey, index: true, foreign_key: true)
drop_table :gpg_key_subkeys
end
end
# See http://doc.gitlab.com/ce/development/migration_style_guide.html
# for more information on how to write migrations for GitLab.
class ScheduleCreateGpgKeySubkeysFromGpgKeys < ActiveRecord::Migration
disable_ddl_transaction!
DOWNTIME = false
MIGRATION = 'CreateGpgKeySubkeysFromGpgKeys'
class GpgKey < ActiveRecord::Base
self.table_name = 'gpg_keys'
include EachBatch
end
def up
GpgKey.select(:id).each_batch do |gpg_keys|
jobs = gpg_keys.pluck(:id).map do |id|
[MIGRATION, [id]]
end
BackgroundMigrationWorker.perform_bulk(jobs)
end
end
def down
end
end
......@@ -11,7 +11,7 @@
#
# It's strongly recommended that you check this file into your version control system.
ActiveRecord::Schema.define(version: 20171004121444) do
ActiveRecord::Schema.define(version: 20171005130944) do
# These are extensions that must be enabled in order to support this database
enable_extension "plpgsql"
......@@ -580,6 +580,16 @@
add_index "forked_project_links", ["forked_to_project_id"], name: "index_forked_project_links_on_forked_to_project_id", unique: true, using: :btree
create_table "gpg_key_subkeys", force: :cascade do |t|
t.integer "gpg_key_id", null: false
t.binary "keyid"
t.binary "fingerprint"
end
add_index "gpg_key_subkeys", ["fingerprint"], name: "index_gpg_key_subkeys_on_fingerprint", unique: true, using: :btree
add_index "gpg_key_subkeys", ["gpg_key_id"], name: "index_gpg_key_subkeys_on_gpg_key_id", using: :btree
add_index "gpg_key_subkeys", ["keyid"], name: "index_gpg_key_subkeys_on_keyid", unique: true, using: :btree
create_table "gpg_keys", force: :cascade do |t|
t.datetime_with_timezone "created_at", null: false
t.datetime_with_timezone "updated_at", null: false
......@@ -603,11 +613,13 @@
t.text "gpg_key_user_name"
t.text "gpg_key_user_email"
t.integer "verification_status", limit: 2, default: 0, null: false
t.integer "gpg_key_subkey_id"
end
add_index "gpg_signatures", ["commit_sha"], name: "index_gpg_signatures_on_commit_sha", unique: true, using: :btree
add_index "gpg_signatures", ["gpg_key_id"], name: "index_gpg_signatures_on_gpg_key_id", using: :btree
add_index "gpg_signatures", ["gpg_key_primary_keyid"], name: "index_gpg_signatures_on_gpg_key_primary_keyid", using: :btree
add_index "gpg_signatures", ["gpg_key_subkey_id"], name: "index_gpg_signatures_on_gpg_key_subkey_id", using: :btree
add_index "gpg_signatures", ["project_id"], name: "index_gpg_signatures_on_project_id", using: :btree
create_table "identities", force: :cascade do |t|
......@@ -1727,7 +1739,9 @@
add_foreign_key "events", "projects", on_delete: :cascade
add_foreign_key "events", "users", column: "author_id", name: "fk_edfd187b6f", on_delete: :cascade
add_foreign_key "forked_project_links", "projects", column: "forked_to_project_id", name: "fk_434510edb0", on_delete: :cascade
add_foreign_key "gpg_key_subkeys", "gpg_keys", on_delete: :cascade
add_foreign_key "gpg_keys", "users", on_delete: :cascade
add_foreign_key "gpg_signatures", "gpg_key_subkeys", on_delete: :nullify
add_foreign_key "gpg_signatures", "gpg_keys", on_delete: :nullify
add_foreign_key "gpg_signatures", "projects", on_delete: :cascade
add_foreign_key "issue_assignees", "issues", name: "fk_b7d881734a", on_delete: :cascade
......
class Gitlab::BackgroundMigration::CreateGpgKeySubkeysFromGpgKeys
class GpgKey < ActiveRecord::Base
self.table_name = 'gpg_keys'
include EachBatch
include ShaAttribute
sha_attribute :primary_keyid
sha_attribute :fingerprint
has_many :subkeys, class_name: 'GpgKeySubkey'
end
class GpgKeySubkey < ActiveRecord::Base
self.table_name = 'gpg_key_subkeys'
include ShaAttribute
sha_attribute :keyid
sha_attribute :fingerprint
end
def perform(gpg_key_id)
gpg_key = GpgKey.find_by(id: gpg_key_id)
return if gpg_key.nil?
return if gpg_key.subkeys.any?
create_subkeys(gpg_key)
update_signatures(gpg_key)
end
private
def create_subkeys(gpg_key)
gpg_subkeys = Gitlab::Gpg.subkeys_from_key(gpg_key.key)
gpg_subkeys[gpg_key.primary_keyid.upcase]&.each do |subkey_data|
gpg_key.subkeys.build(keyid: subkey_data[:keyid], fingerprint: subkey_data[:fingerprint])
end
# Improve latency by doing all INSERTs in a single call
GpgKey.transaction do
gpg_key.save!
end
end
def update_signatures(gpg_key)
return unless gpg_key.subkeys.exists?
InvalidGpgSignatureUpdateWorker.perform_async(gpg_key.id)
end
end
......@@ -34,6 +34,21 @@ def primary_keyids_from_key(key)
end
end
def subkeys_from_key(key)
using_tmp_keychain do
fingerprints = CurrentKeyChain.fingerprints_from_key(key)
raw_keys = GPGME::Key.find(:public, fingerprints)
raw_keys.each_with_object({}) do |raw_key, grouped_subkeys|
primary_subkey_id = raw_key.primary_subkey.keyid
grouped_subkeys[primary_subkey_id] = raw_key.subkeys[1..-1].map do |s|
{ keyid: s.keyid, fingerprint: s.fingerprint }
end
end
end
end
def user_infos_from_key(key)
using_tmp_keychain do
fingerprints = CurrentKeyChain.fingerprints_from_key(key)
......
......@@ -43,7 +43,9 @@ def using_keychain
# key belonging to the keyid.
# This way we can add the key to the temporary keychain and extract
# the proper signature.
gpg_key = GpgKey.find_by(primary_keyid: verified_signature.fingerprint)
# 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)
if gpg_key
Gitlab::Gpg::CurrentKeyChain.add(gpg_key.key)
......@@ -74,7 +76,7 @@ def attributes(gpg_key)
commit_sha: @commit.sha,
project: @commit.project,
gpg_key: gpg_key,
gpg_key_primary_keyid: gpg_key&.primary_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
......@@ -98,6 +100,10 @@ def verification_status(gpg_key)
def user_infos(gpg_key)
gpg_key&.verified_user_infos&.first || gpg_key&.user_infos&.first || {}
end
def find_gpg_key(keyid)
GpgKey.find_by(primary_keyid: keyid) || GpgKeySubkey.find_by(keyid: keyid)
end
end
end
end
......@@ -9,7 +9,7 @@ def run
GpgSignature
.select(:id, :commit_sha, :project_id)
.where('gpg_key_id IS NULL OR verification_status <> ?', GpgSignature.verification_statuses[:verified])
.where(gpg_key_primary_keyid: @gpg_key.primary_keyid)
.where(gpg_key_primary_keyid: @gpg_key.keyids)
.find_each { |sig| sig.gpg_commit.update_signature!(sig) }
end
end
......
require_relative '../support/gpg_helpers'
FactoryGirl.define do
factory :gpg_key_subkey do
gpg_key
sequence(:keyid) { |n| "keyid-#{n}" }
sequence(:fingerprint) { |n| "fingerprint-#{n}" }
end
end
......@@ -4,5 +4,9 @@
factory :gpg_key do
key GpgHelpers::User1.public_key
user
factory :gpg_key_with_subkeys do
key GpgHelpers::User1.public_key_with_extra_signing_key
end
end
end
......@@ -5,7 +5,7 @@
commit_sha { Digest::SHA1.hexdigest(SecureRandom.hex) }
project
gpg_key
gpg_key_primary_keyid { gpg_key.primary_keyid }
gpg_key_primary_keyid { gpg_key.keyid }
verification_status :verified
end
end
......@@ -20,6 +20,18 @@
expect(page).to have_content('bette.cartwright@example.net Unverified')
expect(page).to have_content(GpgHelpers::User2.fingerprint)
end
scenario 'with multiple subkeys' do
fill_in('Key', with: GpgHelpers::User3.public_key)
click_button('Add key')
expect(page).to have_content('john.doe@example.com Unverified')
expect(page).to have_content(GpgHelpers::User3.fingerprint)
GpgHelpers::User3.subkey_fingerprints.each do |fingerprint|
expect(page).to have_content(fingerprint)
end
end
end
scenario 'User sees their key' do
......
require 'spec_helper'
describe Gitlab::BackgroundMigration::CreateGpgKeySubkeysFromGpgKeys, :migration, schema: 20171005130944 do
context 'when GpgKey exists' do
let!(:gpg_key) { create(:gpg_key, key: GpgHelpers::User3.public_key) }
before do
GpgKeySubkey.destroy_all
end
it 'generate the subkeys' do
expect do
described_class.new.perform(gpg_key.id)
end.to change { gpg_key.subkeys.count }.from(0).to(2)
end
it 'schedules the signature update worker' do
expect(InvalidGpgSignatureUpdateWorker).to receive(:perform_async).with(gpg_key.id)
described_class.new.perform(gpg_key.id)
end
end
context 'when GpgKey does not exist' do
it 'does not do anything' do
expect(Gitlab::Gpg).not_to receive(:subkeys_from_key)
expect(InvalidGpgSignatureUpdateWorker).not_to receive(:perform_async)
described_class.new.perform(123)
end
end
end
......@@ -63,6 +63,45 @@
it_behaves_like 'returns the cached signature on second call'
end
context 'commit signed with a subkey' do
let!(:commit) { create :commit, project: project, sha: commit_sha, committer_email: GpgHelpers::User3.emails.first }
let!(:user) { create(:user, email: GpgHelpers::User3.emails.first) }
let!(:gpg_key) do
create :gpg_key, key: GpgHelpers::User3.public_key, user: user
end
let(:gpg_key_subkey) do
gpg_key.subkeys.find_by(fingerprint: '0522DD29B98F167CD8421752E38FFCAF75ABD92A')
end
before do
allow(Rugged::Commit).to receive(:extract_signature)
.with(Rugged::Repository, commit_sha)
.and_return(
[
GpgHelpers::User3.signed_commit_signature,
GpgHelpers::User3.signed_commit_base_data
]
)
end
it 'returns a valid signature' do
expect(described_class.new(commit).signature).to have_attributes(
commit_sha: commit_sha,
project: project,
gpg_key: gpg_key_subkey,
gpg_key_primary_keyid: gpg_key_subkey.keyid,
gpg_key_user_name: GpgHelpers::User3.names.first,
gpg_key_user_email: GpgHelpers::User3.emails.first,
verification_status: 'verified'
)
end
it_behaves_like 'returns the cached signature on second call'
end
context 'user email does not match the committer email, but is the same user' do
let!(:commit) { create :commit, project: project, sha: commit_sha, committer_email: GpgHelpers::User2.emails.first }
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment