From 705998afdaba24bf1c5c9997b1d9f9153b7db8bc Mon Sep 17 00:00:00 2001 From: Igor Drozdov Date: Fri, 6 Jan 2023 18:21:53 +0100 Subject: [PATCH] Allow revoking signing SSH keys Along with removing a key, now it's possible to revoke a key Revoking marks all the commits signed with this key as unverified Changelog: added --- app/controllers/profiles/keys_controller.rb | 10 +++ app/helpers/ssh_keys_helper.rb | 22 ++++++ app/models/concerns/commit_signature.rb | 4 +- app/models/key.rb | 6 ++ app/services/keys/revoke_service.rb | 28 +++++++ app/views/profiles/keys/_key.html.haml | 12 ++- .../_revoked_key_signature_badge.html.haml | 5 ++ .../shared/ssh_keys/_key_delete.html.haml | 6 +- .../development/revoke_ssh_signatures.yml | 8 ++ config/routes/profile.rb | 8 +- doc/api/graphql/reference/index.md | 1 + .../repository/ssh_signed_commits/index.md | 15 +++- ee/spec/features/admin/admin_users_spec.rb | 6 +- locale/gitlab.pot | 18 +++++ spec/features/profiles/keys_spec.rb | 73 ++++++++++++++----- .../verification_status_enum_spec.rb | 2 +- spec/models/key_spec.rb | 8 ++ .../requests/profiles/keys_controller_spec.rb | 31 ++++++++ spec/services/keys/revoke_service_spec.rb | 48 ++++++++++++ .../profiles/keys/_key.html.haml_spec.rb | 32 ++++++-- .../ssh_keys/_key_delete.html.haml_spec.rb | 18 +---- 21 files changed, 307 insertions(+), 54 deletions(-) create mode 100644 app/services/keys/revoke_service.rb create mode 100644 app/views/projects/commit/_revoked_key_signature_badge.html.haml create mode 100644 config/feature_flags/development/revoke_ssh_signatures.yml create mode 100644 spec/requests/profiles/keys_controller_spec.rb create mode 100644 spec/services/keys/revoke_service_spec.rb diff --git a/app/controllers/profiles/keys_controller.rb b/app/controllers/profiles/keys_controller.rb index 39e8f6c500d5bb..05c3aa0dd3b8b9 100644 --- a/app/controllers/profiles/keys_controller.rb +++ b/app/controllers/profiles/keys_controller.rb @@ -34,6 +34,16 @@ def destroy end end + def revoke + @key = current_user.keys.find(params[:id]) + Keys::RevokeService.new(current_user).execute(@key) + + respond_to do |format| + format.html { redirect_to profile_keys_url, status: :found } + format.js { head :ok } + end + end + private def key_params diff --git a/app/helpers/ssh_keys_helper.rb b/app/helpers/ssh_keys_helper.rb index 4cd40836335065..13d6851f3cd204 100644 --- a/app/helpers/ssh_keys_helper.rb +++ b/app/helpers/ssh_keys_helper.rb @@ -23,6 +23,28 @@ def ssh_key_delete_modal_data(key, path) } end + def ssh_key_revoke_modal_data(key, path) + title = _('Revoke Key') + + { + path: path, + method: 'delete', + qa_selector: 'revoke_ssh_key_button', + title: title, + aria_label: title, + modal_attributes: { + 'data-qa-selector': 'ssh_key_revoke_modal', + title: _('Are you sure you want to revoke this SSH key?'), + message: _('This action cannot be undone, and will permanently delete the %{key} SSH key. All commits signed using this SSH key will be marked as unverified.') % { key: key.title }, + okVariant: 'danger', + okTitle: _('Revoke') + }, + toggle: 'tooltip', + placement: 'top', + container: 'body' + } + end + def ssh_key_allowed_algorithms allowed_algorithms = Gitlab::CurrentSettings.allowed_key_types.flat_map do |ssh_key_type_name| Gitlab::SSHPublicKey.supported_algorithms_for_name(ssh_key_type_name) diff --git a/app/models/concerns/commit_signature.rb b/app/models/concerns/commit_signature.rb index 7f1fbbefd9421c..5dac3c7833a305 100644 --- a/app/models/concerns/commit_signature.rb +++ b/app/models/concerns/commit_signature.rb @@ -4,6 +4,7 @@ module CommitSignature included do include ShaAttribute + include EachBatch sha_attribute :commit_sha @@ -14,7 +15,8 @@ module CommitSignature other_user: 3, unverified_key: 4, unknown_key: 5, - multiple_signatures: 6 + multiple_signatures: 6, + revoked_key: 7 } belongs_to :project, class_name: 'Project', foreign_key: 'project_id', optional: false diff --git a/app/models/key.rb b/app/models/key.rb index 1f2234129eda84..596186276bb409 100644 --- a/app/models/key.rb +++ b/app/models/key.rb @@ -11,6 +11,8 @@ class Key < ApplicationRecord belongs_to :user + has_many :ssh_signatures, class_name: 'CommitSignatures::SshSignature' + before_validation :generate_fingerprint validates :title, @@ -136,6 +138,10 @@ def ensure_sha256_fingerprint! save if generate_fingerprint end + def signing? + super || auth_and_signing? + end + private def generate_fingerprint diff --git a/app/services/keys/revoke_service.rb b/app/services/keys/revoke_service.rb new file mode 100644 index 00000000000000..42ea9ab73bef44 --- /dev/null +++ b/app/services/keys/revoke_service.rb @@ -0,0 +1,28 @@ +# frozen_string_literal: true + +module Keys + class RevokeService < ::Keys::DestroyService + def execute(key) + key.transaction do + unverify_associated_signatures(key) + + raise ActiveRecord::Rollback unless super(key) + end + end + + private + + def unverify_associated_signatures(key) + return unless Feature.enabled?(:revoke_ssh_signatures) + + key.ssh_signatures.each_batch do |batch| + batch.update_all( + verification_status: CommitSignatures::SshSignature.verification_statuses[:revoked_key], + updated_at: Time.zone.now + ) + end + end + end +end + +Keys::DestroyService.prepend_mod diff --git a/app/views/profiles/keys/_key.html.haml b/app/views/profiles/keys/_key.html.haml index 219e7c4d2fe5d3..b4b205bc52a10a 100644 --- a/app/views/profiles/keys/_key.html.haml +++ b/app/views/profiles/keys/_key.html.haml @@ -28,7 +28,11 @@ %span.last-used-at.gl-mr-3 = s_('Profiles|Usage type:') = ssh_key_usage_types.invert[key.usage_type] - %span.key-created-at.gl-display-flex.gl-align-items-center - - if key.can_delete? - .gl-ml-3 - = render 'shared/ssh_keys/key_delete', icon: true, button_data: ssh_key_delete_modal_data(key, path_to_key(key, is_admin)) + .gl-display-flex.gl-float-right + - if key.can_delete? + - if key.signing? && !is_admin && Feature.enabled?(:revoke_ssh_signatures) + = render Pajamas::ButtonComponent.new(size: :small, button_options: { class: 'js-confirm-modal-button', data: ssh_key_revoke_modal_data(key, revoke_profile_key_path(key)) }) do + = _('Revoke') + .gl-pl-3 + = render Pajamas::ButtonComponent.new(size: :small, button_options: { class: 'js-confirm-modal-button', data: ssh_key_delete_modal_data(key, path_to_key(key, is_admin)) }) do + = _('Remove') diff --git a/app/views/projects/commit/_revoked_key_signature_badge.html.haml b/app/views/projects/commit/_revoked_key_signature_badge.html.haml new file mode 100644 index 00000000000000..2e0ca42561a6c1 --- /dev/null +++ b/app/views/projects/commit/_revoked_key_signature_badge.html.haml @@ -0,0 +1,5 @@ +- title = s_('CommitSignature|Unverified signature') +- description = s_('CommitSignature|This commit was signed with a key that was revoked.') +- locals = { signature: signature, title: title, description: description, label: s_('CommitSignature|Unverified'), css_class: 'invalid' } + += render partial: 'projects/commit/signature_badge', locals: locals diff --git a/app/views/shared/ssh_keys/_key_delete.html.haml b/app/views/shared/ssh_keys/_key_delete.html.haml index 4b89b2a0cbfa1e..80cd23989a05f1 100644 --- a/app/views/shared/ssh_keys/_key_delete.html.haml +++ b/app/views/shared/ssh_keys/_key_delete.html.haml @@ -1,7 +1,5 @@ -- icon = local_assigns[:icon] - category = local_assigns[:category] || :primary .gl-p-2 - = render Pajamas::ButtonComponent.new(variant: :danger, category: category, icon: ('remove' if icon), button_options: { class: 'js-confirm-modal-button', data: button_data }) do - - unless icon - = _('Delete') + = render Pajamas::ButtonComponent.new(variant: :danger, category: category, button_options: { class: 'js-confirm-modal-button', data: button_data }) do + = _('Delete') diff --git a/config/feature_flags/development/revoke_ssh_signatures.yml b/config/feature_flags/development/revoke_ssh_signatures.yml new file mode 100644 index 00000000000000..6232e6995152f8 --- /dev/null +++ b/config/feature_flags/development/revoke_ssh_signatures.yml @@ -0,0 +1,8 @@ +--- +name: revoke_ssh_signatures +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/108344 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/388986 +milestone: '15.9' +type: development +group: group::source code +default_enabled: false diff --git a/config/routes/profile.rb b/config/routes/profile.rb index 91f6eb678e414c..46a078ce3b1a88 100644 --- a/config/routes/profile.rb +++ b/config/routes/profile.rb @@ -38,7 +38,13 @@ end end resource :preferences, only: [:show, :update] - resources :keys, only: [:index, :show, :create, :destroy] + + resources :keys, only: [:index, :show, :create, :destroy] do + member do + delete :revoke + end + end + resources :gpg_keys, only: [:index, :create, :destroy] do member do put :revoke diff --git a/doc/api/graphql/reference/index.md b/doc/api/graphql/reference/index.md index ef6ad4000d4f1a..56e160218fb127 100644 --- a/doc/api/graphql/reference/index.md +++ b/doc/api/graphql/reference/index.md @@ -23189,6 +23189,7 @@ Verification status of a GPG or X.509 signature for a commit. | ----- | ----------- | | `MULTIPLE_SIGNATURES` | multiple_signatures verification status. | | `OTHER_USER` | other_user verification status. | +| `REVOKED_KEY` | revoked_key verification status. | | `SAME_USER_DIFFERENT_EMAIL` | same_user_different_email verification status. | | `UNKNOWN_KEY` | unknown_key verification status. | | `UNVERIFIED` | unverified verification status. | diff --git a/doc/user/project/repository/ssh_signed_commits/index.md b/doc/user/project/repository/ssh_signed_commits/index.md index 85d2ce1d4801cf..4a6a6ebcdba9ec 100644 --- a/doc/user/project/repository/ssh_signed_commits/index.md +++ b/doc/user/project/repository/ssh_signed_commits/index.md @@ -160,8 +160,19 @@ for Git to associate SSH public keys with users: ## Revoke an SSH key for signing commits -You can't revoke an SSH key used for signing commits. To learn more, read -[Add revocation for SSH keys](https://gitlab.com/gitlab-org/gitlab/-/issues/382984). +> [Introduced](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/108344) in GitLab 15.9. + +If an SSH key becomes compromised, revoke it. Revoking a key changes both future and past commits: + +- Past commits signed by this key are marked as unverified. +- Future commits signed by this key are marked as unverified. + +To revoke an SSH key: + +1. In the top-right corner, select your avatar. +1. Select **Edit profile**. +1. On the left sidebar, select (**{key}**) **SSH Keys**. +1. Select **Revoke** next to the SSH key you want to delete. ## Related topics diff --git a/ee/spec/features/admin/admin_users_spec.rb b/ee/spec/features/admin/admin_users_spec.rb index 23d6f1eabcca35..bb5aca93773587 100644 --- a/ee/spec/features/admin/admin_users_spec.rb +++ b/ee/spec/features/admin/admin_users_spec.rb @@ -195,13 +195,15 @@ def visit_edit_user(user_id) # SSH key should be the first in the list within('ul.content-list li.key-list-item:nth-of-type(1)') do expect(page).to have_content(key2.title) - expect(page).to have_selector('[data-testid=remove-icon]') + expect(page).to have_content('Remove') + expect(page).not_to have_content('Revoke') end # Next, LDAP key within('ul.content-list li.key-list-item:nth-of-type(2)') do expect(page).to have_content(key1.title) - expect(page).not_to have_selector('[data-testid=remove-icon]') + expect(page).not_to have_content('Remove') + expect(page).not_to have_content('Revoke') end end end diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 44f131541653d9..83186e97a649be 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -5330,6 +5330,9 @@ msgstr "" msgid "Are you sure you want to revoke this %{accessTokenType}? This action cannot be undone." msgstr "" +msgid "Are you sure you want to revoke this SSH key?" +msgstr "" + msgid "Are you sure you want to revoke this group access token? This action cannot be undone." msgstr "" @@ -10081,6 +10084,15 @@ msgstr "" msgid "CommitMessage|Add %{file_name}" msgstr "" +msgid "CommitSignature|This commit was signed with a key that was revoked." +msgstr "" + +msgid "CommitSignature|Unverified" +msgstr "" + +msgid "CommitSignature|Unverified signature" +msgstr "" + msgid "CommitWidget|authored" msgstr "" @@ -36058,6 +36070,9 @@ msgstr "" msgid "Revoke" msgstr "" +msgid "Revoke Key" +msgstr "" + msgid "Revoked" msgstr "" @@ -42822,6 +42837,9 @@ msgstr "" msgid "This action cannot be undone, and will permanently delete the %{key} SSH key" msgstr "" +msgid "This action cannot be undone, and will permanently delete the %{key} SSH key. All commits signed using this SSH key will be marked as unverified." +msgstr "" + msgid "This action deletes %{codeOpen}%{project_path_with_namespace}%{codeClose} and everything this project contains. %{strongOpen}There is no going back.%{strongClose}" msgstr "" diff --git a/spec/features/profiles/keys_spec.rb b/spec/features/profiles/keys_spec.rb index 7a2a12d8dca0ad..00f43092c62d7d 100644 --- a/spec/features/profiles/keys_spec.rb +++ b/spec/features/profiles/keys_spec.rb @@ -76,35 +76,74 @@ expect(page).to have_content(key.title) end + def destroy_key(path, action, confirmation_button) + visit path + + page.click_button(action) + + page.within('.modal') do + page.click_button(confirmation_button) + end + + expect(page).to have_content('Your SSH keys (0)') + end + describe 'User removes a key', :js do - shared_examples 'removes key' do - it 'removes key' do - visit path - find('[data-testid=remove-icon]').click + let!(:key) { create(:key, user: user) } - page.within('.modal') do - page.click_button('Delete') - end + context 'via the key index' do + it 'removes key' do + destroy_key(profile_keys_path, 'Remove', 'Delete') + end + end - expect(page).to have_content('Your SSH keys (0)') + context 'via its details page' do + it 'removes key' do + destroy_key(profile_keys_path(key), 'Remove', 'Delete') end end + end + + describe 'User revokes a key', :js do + context 'when a commit is signed using SSH key' do + let!(:project) { create(:project, :repository) } + let!(:key) { create(:key, user: user) } + let!(:commit) { project.commit('ssh-signed-commit') } + + let!(:signature) do + create(:ssh_signature, + project: project, + key: key, + key_fingerprint_sha256: key.fingerprint_sha256, + commit_sha: commit.sha) + end - context 'via the key index' do before do - create(:key, user: user) + project.add_developer(user) end - let(:path) { profile_keys_path } + it 'revoking the SSH key marks commits as unverified' do + visit project_commit_path(project, commit) - it_behaves_like 'removes key' - end + find('a.gpg-status-box', text: 'Verified').click - context 'via its details page' do - let(:key) { create(:key, user: user) } - let(:path) { profile_keys_path(key) } + within('.popover') do + expect(page).to have_content("Verified commit") + expect(page).to have_content("SSH key fingerprint: #{key.fingerprint_sha256}") + end + + destroy_key(profile_keys_path, 'Revoke', 'Revoke') + + visit project_commit_path(project, commit) - it_behaves_like 'removes key' + find('a.gpg-status-box', text: 'Unverified').click + + within('.popover') do + expect(page).to have_content("Unverified signature") + expect(page).to have_content('This commit was signed with a key that was revoked.') + expect(page).to have_content("SSH key fingerprint: #{signature.key_fingerprint_sha256}") + end + end end end end diff --git a/spec/graphql/types/commit_signatures/verification_status_enum_spec.rb b/spec/graphql/types/commit_signatures/verification_status_enum_spec.rb index cb7ce19c9fc128..a0d99f5f0c10e5 100644 --- a/spec/graphql/types/commit_signatures/verification_status_enum_spec.rb +++ b/spec/graphql/types/commit_signatures/verification_status_enum_spec.rb @@ -10,7 +10,7 @@ .to match_array(%w[ UNVERIFIED UNVERIFIED_KEY VERIFIED SAME_USER_DIFFERENT_EMAIL OTHER_USER UNKNOWN_KEY - MULTIPLE_SIGNATURES + MULTIPLE_SIGNATURES REVOKED_KEY ]) end end diff --git a/spec/models/key_spec.rb b/spec/models/key_spec.rb index 92f4d6d85316bc..f1bc7b41ceeca9 100644 --- a/spec/models/key_spec.rb +++ b/spec/models/key_spec.rb @@ -489,4 +489,12 @@ end end end + + describe '#signing?' do + it 'returns whether a key can be used for signing' do + expect(build(:key, usage_type: :signing)).to be_signing + expect(build(:key, usage_type: :auth_and_signing)).to be_signing + expect(build(:key, usage_type: :auth)).not_to be_signing + end + end end diff --git a/spec/requests/profiles/keys_controller_spec.rb b/spec/requests/profiles/keys_controller_spec.rb new file mode 100644 index 00000000000000..48c382e6230964 --- /dev/null +++ b/spec/requests/profiles/keys_controller_spec.rb @@ -0,0 +1,31 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Profiles::KeysController, feature_category: :source_code_management do + let_it_be(:user) { create(:user) } + + before do + login_as(user) + end + + describe 'DELETE /-/profile/keys/:id/revoke' do + it 'returns 404 if a key not found' do + delete revoke_profile_key_path(non_existing_record_id) + + expect(response).to have_gitlab_http_status(:not_found) + end + + it 'revokes ssh commit signatures' do + key = create(:key, user: user) + signature = create(:ssh_signature, key: key) + + expect do + delete revoke_profile_key_path(signature.key) + end.to change { signature.reload.key }.from(signature.key).to(nil) + .and change { signature.verification_status }.from('verified').to('revoked_key') + + expect(response).to have_gitlab_http_status(:found) + end + end +end diff --git a/spec/services/keys/revoke_service_spec.rb b/spec/services/keys/revoke_service_spec.rb new file mode 100644 index 00000000000000..ec07701b4b723f --- /dev/null +++ b/spec/services/keys/revoke_service_spec.rb @@ -0,0 +1,48 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Keys::RevokeService, feature_category: :source_code_management do + let(:user) { create(:user) } + + subject(:service) { described_class.new(user) } + + it 'destroys a key' do + key = create(:key) + + expect { service.execute(key) }.to change { key.persisted? }.from(true).to(false) + end + + it 'unverifies associated signatures' do + key = create(:key) + signature = create(:ssh_signature, key: key) + + expect do + service.execute(key) + end.to change { signature.reload.key }.from(key).to(nil) + .and change { signature.reload.verification_status }.from('verified').to('revoked_key') + end + + it 'does not unverifies signatures if destroy fails' do + key = create(:key) + signature = create(:ssh_signature, key: key) + + expect(key).to receive(:destroy).and_return(false) + + expect { service.execute(key) }.not_to change { signature.reload.verification_status } + expect(key).to be_persisted + end + + context 'when revoke_ssh_signatures disabled' do + before do + stub_feature_flags(revoke_ssh_signatures: false) + end + + it 'does not unverifies signatures' do + key = create(:key) + signature = create(:ssh_signature, key: key) + + expect { service.execute(key) }.not_to change { signature.reload.verification_status } + end + end +end diff --git a/spec/views/profiles/keys/_key.html.haml_spec.rb b/spec/views/profiles/keys/_key.html.haml_spec.rb index d2e27bd2ee0ce4..09053a29fe0105 100644 --- a/spec/views/profiles/keys/_key.html.haml_spec.rb +++ b/spec/views/profiles/keys/_key.html.haml_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe 'profiles/keys/_key.html.haml' do +RSpec.describe 'profiles/keys/_key.html.haml', feature_category: :authentication_and_authorization do let_it_be(:user) { create(:user) } before do @@ -27,15 +27,18 @@ expect(rendered).to have_text(l(key.last_used_at, format: "%b %d, %Y")) expect(rendered).to have_text(l(key.created_at, format: "%b %d, %Y")) expect(rendered).to have_text(key.expires_at.to_date) - expect(response).to render_template(partial: 'shared/ssh_keys/_key_delete') + expect(rendered).to have_button('Remove') end context 'displays the usage type' do - where(:usage_type, :usage_type_text) do + where(:usage_type, :usage_type_text, :displayed_buttons, :hidden_buttons, :revoke_ssh_signatures_ff) do [ - [:auth, 'Authentication'], - [:auth_and_signing, 'Authentication & Signing'], - [:signing, 'Signing'] + [:auth, 'Authentication', ['Remove'], ['Revoke'], true], + [:auth_and_signing, 'Authentication & Signing', %w[Remove Revoke], [], true], + [:signing, 'Signing', %w[Remove Revoke], [], true], + [:auth, 'Authentication', ['Remove'], ['Revoke'], false], + [:auth_and_signing, 'Authentication & Signing', %w[Remove], ['Revoke'], false], + [:signing, 'Signing', %w[Remove], ['Revoke'], false] ] end @@ -47,6 +50,20 @@ expect(rendered).to have_text(usage_type_text) end + + it 'renders remove/revoke buttons', :aggregate_failures do + stub_feature_flags(revoke_ssh_signatures: revoke_ssh_signatures_ff) + + render + + displayed_buttons.each do |button| + expect(rendered).to have_text(button) + end + + hidden_buttons.each do |button| + expect(rendered).not_to have_text(button) + end + end end end @@ -98,7 +115,8 @@ it 'does not render the partial' do render - expect(response).not_to render_template(partial: 'shared/ssh_keys/_key_delete') + expect(response).not_to have_text('Remove') + expect(response).not_to have_text('Revoke') end end diff --git a/spec/views/shared/ssh_keys/_key_delete.html.haml_spec.rb b/spec/views/shared/ssh_keys/_key_delete.html.haml_spec.rb index c9bdcabb4b6f0c..5cef3a949d32ec 100644 --- a/spec/views/shared/ssh_keys/_key_delete.html.haml_spec.rb +++ b/spec/views/shared/ssh_keys/_key_delete.html.haml_spec.rb @@ -2,21 +2,9 @@ require 'spec_helper' RSpec.describe 'shared/ssh_keys/_key_delete.html.haml' do - context 'when the icon parameter is used' do - it 'has text' do - render partial: 'shared/ssh_keys/key_delete', formats: :html, locals: { icon: true, button_data: '' } + it 'has text' do + render partial: 'shared/ssh_keys/key_delete', formats: :html, locals: { button_data: '' } - expect(rendered).not_to have_button('Delete') - expect(rendered).to have_selector('[data-testid=remove-icon]') - end - end - - context 'when the icon parameter is not used' do - it 'does not have text' do - render partial: 'shared/ssh_keys/key_delete', formats: :html, locals: { button_data: '' } - - expect(rendered).to have_button('Delete') - expect(rendered).not_to have_selector('[data-testid=remove-icon]') - end + expect(rendered).to have_button('Delete') end end -- GitLab