Skip to content

G
gitlab-ce
Commit f82767db authored by Nick Thomas's avatar Nick Thomas
Browse files
Options

Separate actions on authorized keys from gitlab-shell 

The gitlab-shell project used to contain code to manage authorized_keys
but this was long ago merged into the main gitlab project. However, we
still indirect through `Gitlab::Shell` for all key actions. This is a
barrier to understanding and makes things unnecessarily complex. Since
we want to remove `Gitlab::Shell` in the near(ish) future, it also gets
in the way.

This commit removes that indirection and introduces a separate worker
to handle add_key / remove_key calls while maintaining backward
compatibility with in-flight sidekiq jobs.
parent f581927b
Hide whitespace changes
Inline Side-by-side
Showing with 199 additions and 372 deletions
app/models/key.rb
View file @ f82767db
......@@ -30,10 +30,10 @@ class Key < ApplicationRecord
delegate :name, :email, to: :user, prefix: true
after_commit :add_to_shell, on: :create
after_commit :add_to_authorized_keys, on: :create
after_create :post_create_hook
after_create :refresh_user_cache
after_commit :remove_from_shell, on: :destroy
after_commit :remove_from_authorized_keys, on: :destroy
after_destroy :post_destroy_hook
after_destroy :refresh_user_cache
......@@ -79,12 +79,10 @@ class Key < ApplicationRecord
end
# rubocop: enable CodeReuse/ServiceClass
def add_to_shell
GitlabShellWorker.perform_async(
:add_key,
shell_id,
key
)
def add_to_authorized_keys
return unless Gitlab::CurrentSettings.authorized_keys_enabled?
AuthorizedKeysWorker.perform_async(:add_key, shell_id, key)
end
# rubocop: disable CodeReuse/ServiceClass
......@@ -93,11 +91,10 @@ class Key < ApplicationRecord
end
# rubocop: enable CodeReuse/ServiceClass
def remove_from_shell
GitlabShellWorker.perform_async(
:remove_key,
shell_id
)
def remove_from_authorized_keys
return unless Gitlab::CurrentSettings.authorized_keys_enabled?
AuthorizedKeysWorker.perform_async(:remove_key, shell_id)
end
# rubocop: disable CodeReuse/ServiceClass
......
app/workers/all_queues.yml
View file @ f82767db
......@@ -836,6 +836,13 @@
:resource_boundary: :unknown
:weight: 1
:idempotent:
- :name: authorized_keys
:feature_category: :source_code_management
:has_external_dependencies:
:urgency: :high
:resource_boundary: :unknown
:weight: 2
:idempotent: true
- :name: authorized_projects
:feature_category: :authentication_and_authorization
:has_external_dependencies:
......
app/workers/authorized_keys_worker.rb 0 → 100644
View file @ f82767db
# frozen_string_literal: true
class AuthorizedKeysWorker
include ApplicationWorker
PERMITTED_ACTIONS = [:add_key, :remove_key].freeze
feature_category :source_code_management
urgency :high
weight 2
idempotent!
def perform(action, *args)
return unless Gitlab::CurrentSettings.authorized_keys_enabled?
case action
when :add_key
authorized_keys.add_key(*args)
when :remove_key
authorized_keys.remove_key(*args)
end
end
private
def authorized_keys
@authorized_keys ||= Gitlab::AuthorizedKeys.new
end
end
app/workers/gitlab_shell_worker.rb
View file @ f82767db
......@@ -9,6 +9,16 @@ class GitlabShellWorker # rubocop:disable Scalability/IdempotentWorker
weight 2
def perform(action, *arg)
# Gitlab::Shell is being removed but we need to continue to process jobs
# enqueued in the previous release, so handle them here.
#
# See https://gitlab.com/gitlab-org/gitlab/-/issues/25095 for more details
if AuthorizedKeysWorker::PERMITTED_ACTIONS.include?(action)
AuthorizedKeysWorker.new.perform(action, *arg)
return
end
Gitlab::GitalyClient::NamespaceService.allow do
gitlab_shell.__send__(action, *arg) # rubocop:disable GitlabSecurity/PublicSend
end
......
changelogs/unreleased/25095-remove-gitlab-shell-indirection-for-authorized-keys.yml 0 → 100644
View file @ f82767db
---
title: Move authorized_keys operations into their own Sidekiq queue
merge_request: 26913
author:
type: changed
config/sidekiq_queues.yml
View file @ f82767db
......@@ -30,6 +30,8 @@
- 1
- - analytics_code_review_metrics
- 1
- - authorized_keys
- 2
- - authorized_projects
- 2
- - auto_devops
......
db/fixtures/development/11_keys.rb
View file @ f82767db
......@@ -5,9 +5,9 @@ require './spec/support/sidekiq_middleware'
# gitlab-shell path set (yet) we need to disable this for these fixtures.
Sidekiq::Testing.disable! do
Gitlab::Seeder.quiet do
# We want to run `add_to_shell` immediately instead of after the commit, so
# We want to run `add_to_authorized_keys` immediately instead of after the commit, so
# that it falls under `Sidekiq::Testing.disable!`.
Key.skip_callback(:commit, :after, :add_to_shell)
Key.skip_callback(:commit, :after, :add_to_authorized_keys)
User.not_mass_generated.first(10).each do |user|
key = "ssh-rsa AAAAB3NzaC1yc2EAAAABJQAAAIEAiPWx6WM4lhHNedGfBpPJNPpZ7yKu+dnn1SJejgt#{user.id + 100}6k6YjzGGphH2TUxwKzxcKDKKezwkpfnxPkSMkuEspGRt/aZZ9wa++Oi7Qkr8prgHc4soW6NUlfDzpvZK2H5E7eQaSeP3SAwGmQKUFHCddNaP0L+hM7zhFNzjFvpaMgJw0="
......@@ -18,7 +18,7 @@ Sidekiq::Testing.disable! do
)
Sidekiq::Worker.skipping_transaction_check do
key.add_to_shell
key.add_to_authorized_keys
end
print '.'
......
lib/gitlab/authorized_keys.rb
View file @ f82767db
......@@ -70,7 +70,7 @@ module Gitlab
#
# @param [String] id identifier of the key to be removed prefixed by `key-`
# @return [Boolean]
def rm_key(id)
def remove_key(id)
lock do
logger.info("Removing key (#{id})")
open_authorized_keys_file('r+') do |f|
......
lib/gitlab/shell.rb
View file @ f82767db
......@@ -192,84 +192,6 @@ module Gitlab
false
end
# Add new key to authorized_keys
#
# @example Add new key
# add_key("key-42", "sha-rsa ...")
#
# @param [String] key_id identifier of the key
# @param [String] key_content key content (public certificate)
# @return [Boolean] whether key could be added
def add_key(key_id, key_content)
return unless self.authorized_keys_enabled?
gitlab_authorized_keys.add_key(key_id, key_content)
end
# Batch-add keys to authorized_keys
#
# @example
# batch_add_keys(Key.all)
#
# @param [Array<Key>] keys
# @return [Boolean] whether keys could be added
def batch_add_keys(keys)
return unless self.authorized_keys_enabled?
gitlab_authorized_keys.batch_add_keys(keys)
end
# Remove SSH key from authorized_keys
#
# @example Remove a key
# remove_key("key-342")
#
# @param [String] key_id
# @return [Boolean] whether key could be removed or not
def remove_key(key_id, _ = nil)
return unless self.authorized_keys_enabled?
gitlab_authorized_keys.rm_key(key_id)
end
# Remove all SSH keys from gitlab shell
#
# @example Remove all keys
# remove_all_keys
#
# @return [Boolean] whether keys could be removed or not
def remove_all_keys
return unless self.authorized_keys_enabled?
gitlab_authorized_keys.clear
end
# Remove SSH keys from gitlab shell that are not in the DB
#
# @example Remove keys not on the database
# remove_keys_not_found_in_db
#
# rubocop: disable CodeReuse/ActiveRecord
def remove_keys_not_found_in_db
return unless self.authorized_keys_enabled?
Rails.logger.info("Removing keys not found in DB") # rubocop:disable Gitlab/RailsLogger
batch_read_key_ids do |ids_in_file|
ids_in_file.uniq!
keys_in_db = Key.where(id: ids_in_file)
next unless ids_in_file.size > keys_in_db.count # optimization
ids_to_remove = ids_in_file - keys_in_db.pluck(:id)
ids_to_remove.each do |id|
Rails.logger.info("Removing key-#{id} not found in DB") # rubocop:disable Gitlab/RailsLogger
remove_key("key-#{id}")
end
end
end
# rubocop: enable CodeReuse/ActiveRecord
# Add empty directory for storing repositories
#
# @example Add new namespace directory
......@@ -372,14 +294,6 @@ module Gitlab
File.join(Gitlab.config.repositories.storages[storage].legacy_disk_path, dir_name)
end
def authorized_keys_enabled?
# Return true if nil to ensure the authorized_keys methods work while
# fixing the authorized_keys file during migration.
return true if Gitlab::CurrentSettings.current_application_settings.authorized_keys_enabled.nil?
Gitlab::CurrentSettings.current_application_settings.authorized_keys_enabled
end
private
def git_timeout
......@@ -394,32 +308,6 @@ module Gitlab
raise Error, e
end
def gitlab_authorized_keys
@gitlab_authorized_keys ||= Gitlab::AuthorizedKeys.new
end
def batch_read_key_ids(batch_size: 100, &block)
return unless self.authorized_keys_enabled?
gitlab_authorized_keys.list_key_ids.lazy.each_slice(batch_size) do |key_ids|
yield(key_ids)
end
end
def strip_key(key)
key.split(/[ ]+/)[0, 2].join(' ')
end
def add_keys_to_io(keys, io)
keys.each do |k|
key = strip_key(k.key)
raise Error.new("Invalid key: #{key.inspect}") if key.include?("\t") || key.include?("\n")
io.puts("#{k.shell_id}\t#{key}")
end
end
class GitalyGitlabProjects
attr_reader :shard_name, :repository_relative_path, :output, :gl_project_path
......
lib/tasks/gitlab/shell.rake
View file @ f82767db
......@@ -89,10 +89,12 @@ namespace :gitlab do
puts ""
end
Gitlab::Shell.new.remove_all_keys
authorized_keys = Gitlab::AuthorizedKeys.new
authorized_keys.clear
Key.find_in_batches(batch_size: 1000) do |keys|
unless Gitlab::Shell.new.batch_add_keys(keys)
unless authorized_keys.batch_add_keys(keys)
puts "Failed to add keys...".color(:red)
exit 1
end
......@@ -103,7 +105,7 @@ namespace :gitlab do
end
def ensure_write_to_authorized_keys_is_enabled
return if Gitlab::CurrentSettings.current_application_settings.authorized_keys_enabled
return if Gitlab::CurrentSettings.authorized_keys_enabled?
puts authorized_keys_is_disabled_warning
......@@ -113,7 +115,7 @@ namespace :gitlab do
end
puts 'Enabling the "Write to authorized_keys file" setting...'
Gitlab::CurrentSettings.current_application_settings.update!(authorized_keys_enabled: true)
Gitlab::CurrentSettings.update!(authorized_keys_enabled: true)
puts 'Successfully enabled "Write to authorized_keys file"!'
puts ''
......
spec/lib/gitlab/authorized_keys_spec.rb
View file @ f82767db
......@@ -162,10 +162,10 @@ describe Gitlab::AuthorizedKeys do
end
end
describe '#rm_key' do
describe '#remove_key' do
let(:key) { 'key-741' }
subject { authorized_keys.rm_key(key) }
subject { authorized_keys.remove_key(key) }
context 'authorized_keys file exists' do
let(:other_line) { "command=\"#{Gitlab.config.gitlab_shell.path}/bin/gitlab-shell key-742\",options ssh-rsa AAAAB3NzaDAxx2E" }
......
spec/lib/gitlab/shell_spec.rb
View file @ f82767db
......@@ -9,14 +9,11 @@ describe Gitlab::Shell do
let(:gitlab_shell) { described_class.new }
let(:popen_vars) { { 'GIT_TERMINAL_PROMPT' => ENV['GIT_TERMINAL_PROMPT'] } }
let(:timeout) { Gitlab.config.gitlab_shell.git_timeout }
let(:gitlab_authorized_keys) { double }
before do
allow(Project).to receive(:find).and_return(project)
end
it { is_expected.to respond_to :add_key }
it { is_expected.to respond_to :remove_key }
it { is_expected.to respond_to :create_repository }
it { is_expected.to respond_to :remove_repository }
it { is_expected.to respond_to :fork_repository }
......@@ -49,227 +46,6 @@ describe Gitlab::Shell do
end
end
describe '#add_key' do
context 'when authorized_keys_enabled is true' do
it 'calls Gitlab::AuthorizedKeys#add_key with id and key' do
expect(Gitlab::AuthorizedKeys).to receive(:new).and_return(gitlab_authorized_keys)
expect(gitlab_authorized_keys)
.to receive(:add_key)
.with('key-123', 'ssh-rsa foobar')
gitlab_shell.add_key('key-123', 'ssh-rsa foobar')
end
end
context 'when authorized_keys_enabled is false' do
before do
stub_application_setting(authorized_keys_enabled: false)
end
it 'does nothing' do
expect(Gitlab::AuthorizedKeys).not_to receive(:new)
gitlab_shell.add_key('key-123', 'ssh-rsa foobar trailing garbage')
end
end
context 'when authorized_keys_enabled is nil' do
before do
stub_application_setting(authorized_keys_enabled: nil)
end
it 'calls Gitlab::AuthorizedKeys#add_key with id and key' do
expect(Gitlab::AuthorizedKeys).to receive(:new).and_return(gitlab_authorized_keys)
expect(gitlab_authorized_keys)
.to receive(:add_key)
.with('key-123', 'ssh-rsa foobar')
gitlab_shell.add_key('key-123', 'ssh-rsa foobar')
end
end
end
describe '#batch_add_keys' do
let(:keys) { [double(shell_id: 'key-123', key: 'ssh-rsa foobar')] }
context 'when authorized_keys_enabled is true' do
it 'calls Gitlab::AuthorizedKeys#batch_add_keys with keys to be added' do
expect(Gitlab::AuthorizedKeys).to receive(:new).and_return(gitlab_authorized_keys)
expect(gitlab_authorized_keys)
.to receive(:batch_add_keys)
.with(keys)
gitlab_shell.batch_add_keys(keys)
end
end
context 'when authorized_keys_enabled is false' do
before do
stub_application_setting(authorized_keys_enabled: false)
end
it 'does nothing' do
expect(Gitlab::AuthorizedKeys).not_to receive(:new)
gitlab_shell.batch_add_keys(keys)
end
end
context 'when authorized_keys_enabled is nil' do
before do
stub_application_setting(authorized_keys_enabled: nil)
end
it 'calls Gitlab::AuthorizedKeys#batch_add_keys with keys to be added' do
expect(Gitlab::AuthorizedKeys).to receive(:new).and_return(gitlab_authorized_keys)
expect(gitlab_authorized_keys)
.to receive(:batch_add_keys)
.with(keys)
gitlab_shell.batch_add_keys(keys)
end
end
end
describe '#remove_key' do
context 'when authorized_keys_enabled is true' do
it 'calls Gitlab::AuthorizedKeys#rm_key with the key to be removed' do
expect(Gitlab::AuthorizedKeys).to receive(:new).and_return(gitlab_authorized_keys)
expect(gitlab_authorized_keys).to receive(:rm_key).with('key-123')
gitlab_shell.remove_key('key-123')
end
end
context 'when authorized_keys_enabled is false' do
before do
stub_application_setting(authorized_keys_enabled: false)
end
it 'does nothing' do
expect(Gitlab::AuthorizedKeys).not_to receive(:new)
gitlab_shell.remove_key('key-123')
end
end
context 'when authorized_keys_enabled is nil' do
before do
stub_application_setting(authorized_keys_enabled: nil)
end
it 'calls Gitlab::AuthorizedKeys#rm_key with the key to be removed' do
expect(Gitlab::AuthorizedKeys).to receive(:new).and_return(gitlab_authorized_keys)
expect(gitlab_authorized_keys).to receive(:rm_key).with('key-123')
gitlab_shell.remove_key('key-123')
end
end
end
describe '#remove_all_keys' do
context 'when authorized_keys_enabled is true' do
it 'calls Gitlab::AuthorizedKeys#clear' do
expect(Gitlab::AuthorizedKeys).to receive(:new).and_return(gitlab_authorized_keys)
expect(gitlab_authorized_keys).to receive(:clear)
gitlab_shell.remove_all_keys
end
end
context 'when authorized_keys_enabled is false' do
before do
stub_application_setting(authorized_keys_enabled: false)
end
it 'does nothing' do
expect(Gitlab::AuthorizedKeys).not_to receive(:new)
gitlab_shell.remove_all_keys
end
end
context 'when authorized_keys_enabled is nil' do
before do
stub_application_setting(authorized_keys_enabled: nil)
end
it 'calls Gitlab::AuthorizedKeys#clear' do
expect(Gitlab::AuthorizedKeys).to receive(:new).and_return(gitlab_authorized_keys)
expect(gitlab_authorized_keys).to receive(:clear)
gitlab_shell.remove_all_keys
end
end
end
describe '#remove_keys_not_found_in_db' do
context 'when keys are in the file that are not in the DB' do
before do
gitlab_shell.remove_all_keys
gitlab_shell.add_key('key-1234', 'ssh-rsa ASDFASDF')
gitlab_shell.add_key('key-9876', 'ssh-rsa ASDFASDF')
@another_key = create(:key) # this one IS in the DB
end
it 'removes the keys' do
expect(gitlab_shell).to receive(:remove_key).with('key-1234')
expect(gitlab_shell).to receive(:remove_key).with('key-9876')
expect(gitlab_shell).not_to receive(:remove_key).with("key-#{@another_key.id}")
gitlab_shell.remove_keys_not_found_in_db
end
end
context 'when keys there are duplicate keys in the file that are not in the DB' do
before do
gitlab_shell.remove_all_keys
gitlab_shell.add_key('key-1234', 'ssh-rsa ASDFASDF')
gitlab_shell.add_key('key-1234', 'ssh-rsa ASDFASDF')
end
it 'removes the keys' do
expect(gitlab_shell).to receive(:remove_key).with('key-1234')
gitlab_shell.remove_keys_not_found_in_db
end
end
context 'when keys there are duplicate keys in the file that ARE in the DB' do
before do
gitlab_shell.remove_all_keys
@key = create(:key)
gitlab_shell.add_key(@key.shell_id, @key.key)
end
it 'does not remove the key' do
expect(gitlab_shell).not_to receive(:remove_key).with("key-#{@key.id}")
gitlab_shell.remove_keys_not_found_in_db
end
end
unless ENV['CI'] # Skip in CI, it takes 1 minute
context 'when the first batch can be skipped, but the next batch has keys that are not in the DB' do
before do
gitlab_shell.remove_all_keys
100.times { |i| create(:key) } # first batch is all in the DB
gitlab_shell.add_key('key-1234', 'ssh-rsa ASDFASDF')
end
it 'removes the keys not in the DB' do
expect(gitlab_shell).to receive(:remove_key).with('key-1234')
gitlab_shell.remove_keys_not_found_in_db
end
end
end
end
describe 'projects commands' do
let(:gitlab_shell_path) { File.expand_path('tmp/tests/gitlab-shell') }
let(:projects_path) { File.join(gitlab_shell_path, 'bin/gitlab-projects') }
......
spec/models/key_spec.rb
View file @ f82767db
......@@ -181,16 +181,49 @@ describe Key, :mailer do
end
context 'callbacks' do
it 'adds new key to authorized_file' do
key = build(:personal_key, id: 7)
expect(GitlabShellWorker).to receive(:perform_async).with(:add_key, key.shell_id, key.key)
key.save!
let(:key) { build(:personal_key) }
context 'authorized keys file is enabled' do
before do
stub_application_setting(authorized_keys_enabled: true)
end
it 'adds new key to authorized_file' do
allow(AuthorizedKeysWorker).to receive(:perform_async)
key.save!
# Check after the fact so we have access to Key#id
expect(AuthorizedKeysWorker).to have_received(:perform_async).with(:add_key, key.shell_id, key.key)
end
it 'removes key from authorized_file' do
key.save!
expect(AuthorizedKeysWorker).to receive(:perform_async).with(:remove_key, key.shell_id)
key.destroy
end
end
it 'removes key from authorized_file' do
key = create(:personal_key)
expect(GitlabShellWorker).to receive(:perform_async).with(:remove_key, key.shell_id)
key.destroy
context 'authorized_keys file is disabled' do
before do
stub_application_setting(authorized_keys_enabled: false)
end
it 'does not add the key on creation' do
expect(AuthorizedKeysWorker).not_to receive(:perform_async)
key.save!
end
it 'does not remove the key on destruction' do
key.save!
expect(AuthorizedKeysWorker).not_to receive(:perform_async)
key.destroy
end
end
end
......
spec/workers/authorized_keys_worker_spec.rb 0 → 100644
View file @ f82767db
# frozen_string_literal: true
require 'spec_helper'
describe AuthorizedKeysWorker do
let(:worker) { described_class.new }
describe '#perform' do
context 'authorized_keys is enabled' do
before do
stub_application_setting(authorized_keys_enabled: true)
end
describe '#add_key' do
it 'delegates to Gitlab::AuthorizedKeys' do
expect_next_instance_of(Gitlab::AuthorizedKeys) do |instance|
expect(instance).to receive(:add_key).with('foo', 'bar')
end
worker.perform(:add_key, 'foo', 'bar')
end
end
describe '#remove_key' do
it 'delegates to Gitlab::AuthorizedKeys' do
expect_next_instance_of(Gitlab::AuthorizedKeys) do |instance|
expect(instance).to receive(:remove_key).with('foo', 'bar')
end
worker.perform(:remove_key, 'foo', 'bar')
end
end
describe 'all other commands' do
it 'does nothing' do
expect(Gitlab::AuthorizedKeys).not_to receive(:new)
worker.perform(:foo, 'bar', 'baz')
end
end
end
context 'authorized_keys is disabled' do
before do
stub_application_setting(authorized_keys_enabled: false)
end
it 'does nothing' do
expect(Gitlab::AuthorizedKeys).not_to receive(:new)
worker.perform(:add_key, 'foo', 'bar')
end
end
end
end
spec/workers/gitlab_shell_worker_spec.rb
View file @ f82767db
......@@ -5,12 +5,35 @@ require 'spec_helper'
describe GitlabShellWorker do
let(:worker) { described_class.new }
describe '#perform with add_key' do
it 'calls add_key on Gitlab::Shell' do
expect_next_instance_of(Gitlab::Shell) do |instance|
expect(instance).to receive(:add_key).with('foo', 'bar')
describe '#perform' do
describe '#add_key' do
it 'delegates to Gitlab::AuthorizedKeys' do
expect_next_instance_of(Gitlab::AuthorizedKeys) do |instance|
expect(instance).to receive(:add_key).with('foo', 'bar')
end
worker.perform(:add_key, 'foo', 'bar')
end
end
describe '#remove_key' do
it 'delegates to Gitlab::AuthorizedKeys' do
expect_next_instance_of(Gitlab::AuthorizedKeys) do |instance|
expect(instance).to receive(:remove_key).with('foo', 'bar')
end
worker.perform(:remove_key, 'foo', 'bar')
end
end
describe 'all other commands' do
it 'delegates them to Gitlab::Shell' do
expect_next_instance_of(Gitlab::Shell) do |instance|
expect(instance).to receive(:foo).with('bar', 'baz')
end
worker.perform(:foo, 'bar', 'baz')
end
worker.perform(:add_key, 'foo', 'bar')
end
end
end
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
GitLab Nexedi Edition | About GitLab | About Nexedi | 沪ICP备2021021310号-2 | 沪ICP备2021021310号-7