diff options
18 files changed, 749 insertions, 14 deletions
diff --git a/app/helpers/application_settings_helper.rb b/app/helpers/application_settings_helper.rb index b12ea760668..45f7d29eb05 100644 --- a/app/helpers/application_settings_helper.rb +++ b/app/helpers/application_settings_helper.rb @@ -146,6 +146,7 @@ module ApplicationSettingsHelper :after_sign_up_text, :akismet_api_key, :akismet_enabled, + :authorized_keys_enabled, :auto_devops_enabled, :circuitbreaker_access_retries, :circuitbreaker_check_interval, diff --git a/app/models/application_setting.rb b/app/models/application_setting.rb index 253e213af81..8ab338d873d 100644 --- a/app/models/application_setting.rb +++ b/app/models/application_setting.rb @@ -261,6 +261,7 @@ class ApplicationSetting < ActiveRecord::Base { after_sign_up_text: nil, akismet_enabled: false, + authorized_keys_enabled: true, # TODO default to false if the instance is configured to use AuthorizedKeysCommand container_registry_token_expire_delay: 5, default_artifacts_expire_in: '30 days', default_branch_protection: Settings.gitlab['default_branch_protection'], diff --git a/app/views/admin/application_settings/_form.html.haml b/app/views/admin/application_settings/_form.html.haml index 3e2dbb07a6c..ba4ca88a8a9 100644 --- a/app/views/admin/application_settings/_form.html.haml +++ b/app/views/admin/application_settings/_form.html.haml @@ -775,6 +775,22 @@ = link_to icon('question-circle'), help_page_path('administration/polling') %fieldset + %legend Performance optimization + .form-group + .col-sm-offset-2.col-sm-10 + .checkbox + = f.label :authorized_keys_enabled do + = f.check_box :authorized_keys_enabled + Write to "authorized_keys" file + .help-block + By default, we write to the "authorized_keys" file to support Git + over SSH without additional configuration. GitLab can be optimized + to authenticate SSH keys via the database file. Only uncheck this + if you have configured your OpenSSH server to use the + AuthorizedKeysCommand. Click on the help icon for more details. + = link_to icon('question-circle'), help_page_path('administration/operations/fast_ssh_key_lookup') + + %fieldset %legend User and IP Rate Limits .form-group .col-sm-offset-2.col-sm-10 diff --git a/changelogs/unreleased/jej-backport-authorized-keys-to-ce.yml b/changelogs/unreleased/jej-backport-authorized-keys-to-ce.yml new file mode 100644 index 00000000000..4386c631f59 --- /dev/null +++ b/changelogs/unreleased/jej-backport-authorized-keys-to-ce.yml @@ -0,0 +1,5 @@ +--- +title: Backport fast database lookup of SSH authorized_keys from EE +merge_request: 16014 +author: +type: added diff --git a/db/migrate/20160301174731_add_fingerprint_index.rb b/db/migrate/20160301174731_add_fingerprint_index.rb new file mode 100644 index 00000000000..f2c3d1ba1ea --- /dev/null +++ b/db/migrate/20160301174731_add_fingerprint_index.rb @@ -0,0 +1,17 @@ +# rubocop:disable all +class AddFingerprintIndex < ActiveRecord::Migration + disable_ddl_transaction! + + DOWNTIME = false + + # https://gitlab.com/gitlab-org/gitlab-ee/issues/764 + def change + args = [:keys, :fingerprint] + + if Gitlab::Database.postgresql? + args << { algorithm: :concurrently } + end + + add_index(*args) unless index_exists?(:keys, :fingerprint) + end +end diff --git a/db/migrate/20170531180233_add_authorized_keys_enabled_to_application_settings.rb b/db/migrate/20170531180233_add_authorized_keys_enabled_to_application_settings.rb new file mode 100644 index 00000000000..1d86a531eb3 --- /dev/null +++ b/db/migrate/20170531180233_add_authorized_keys_enabled_to_application_settings.rb @@ -0,0 +1,19 @@ +# See http://doc.gitlab.com/ce/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +class AddAuthorizedKeysEnabledToApplicationSettings < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + # Set this constant to true if this migration requires downtime. + DOWNTIME = false + + disable_ddl_transaction! + + def up + add_column_with_default :application_settings, :authorized_keys_enabled, :boolean, default: true, allow_null: false + end + + def down + remove_column :application_settings, :authorized_keys_enabled + end +end diff --git a/db/schema.rb b/db/schema.rb index 4326ca2d73a..b6cc8bc90fc 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -154,6 +154,7 @@ ActiveRecord::Schema.define(version: 20171230123729) do t.integer "gitaly_timeout_default", default: 55, null: false t.integer "gitaly_timeout_medium", default: 30, null: false t.integer "gitaly_timeout_fast", default: 10, null: false + t.boolean "authorized_keys_enabled", default: true, null: false end create_table "audit_events", force: :cascade do |t| diff --git a/doc/administration/operations/fast_ssh_key_lookup.md b/doc/administration/operations/fast_ssh_key_lookup.md new file mode 100644 index 00000000000..b86168f935a --- /dev/null +++ b/doc/administration/operations/fast_ssh_key_lookup.md @@ -0,0 +1,192 @@ +# Fast lookup of authorized SSH keys in the database + +Regular SSH operations become slow as the number of users grows because OpenSSH +searches for a key to authorize a user via a linear search. In the worst case, +such as when the user is not authorized to access GitLab, OpenSSH will scan the +entire file to search for a key. This can take significant time and disk I/O, +which will delay users attempting to push or pull to a repository. Making +matters worse, if users add or remove keys frequently, the operating system may +not be able to cache the `authorized_keys` file, which causes the disk to be +accessed repeatedly. + +GitLab Shell solves this by providing a way to authorize SSH users via a fast, +indexed lookup in the GitLab database. This page describes how to enable the fast +lookup of authorized SSH keys. + +> **Warning:** OpenSSH version 6.9+ is required because +`AuthorizedKeysCommand` must be able to accept a fingerprint. These +instructions will break installations using older versions of OpenSSH, such as +those included with CentOS 6 as of September 2017. If you want to use this +feature for CentOS 6, follow [the instructions on how to build and install a custom OpenSSH package](#compiling-a-custom-version-of-openssh-for-centos-6) before continuing. + +## Setting up fast lookup via GitLab Shell + +GitLab Shell provides a way to authorize SSH users via a fast, indexed lookup +to the GitLab database. GitLab Shell uses the fingerprint of the SSH key to +check whether the user is authorized to access GitLab. + +Create the directory `/opt/gitlab-shell` first: + +```bash +sudo mkdir -p /opt/gitlab-shell +``` + +Create this file at `/opt/gitlab-shell/authorized_keys`: + +``` +#!/bin/bash + +if [[ "$1" == "git" ]]; then + /opt/gitlab/embedded/service/gitlab-shell/bin/authorized_keys $2 +fi +``` + +Set appropriate ownership and permissions: + +``` +sudo chown root:git /opt/gitlab-shell/authorized_keys +sudo chmod 0650 /opt/gitlab-shell/authorized_keys +``` + +Add the following to `/etc/ssh/sshd_config` or to `/assets/sshd_config` if you +are using Omnibus Docker: + +``` +AuthorizedKeysCommand /opt/gitlab-shell/authorized_keys %u %k +AuthorizedKeysCommandUser git +``` + +Reload OpenSSH: + +```bash +# Debian or Ubuntu installations +sudo service ssh reload + +# CentOS installations +sudo service sshd reload +``` + +Confirm that SSH is working by removing your user's SSH key in the UI, adding a +new one, and attempting to pull a repo. + +> **Warning:** Do not disable writes until SSH is confirmed to be working +perfectly because the file will quickly become out-of-date. + +In the case of lookup failures (which are not uncommon), the `authorized_keys` +file will still be scanned. So git SSH performance will still be slow for many +users as long as a large file exists. + +You can disable any more writes to the `authorized_keys` file by unchecking +`Write to "authorized_keys" file` in the Application Settings of your GitLab +installation. + +![Write to authorized keys setting](img/write_to_authorized_keys_setting.png) + +Again, confirm that SSH is working by removing your user's SSH key in the UI, +adding a new one, and attempting to pull a repo. + +Then you can backup and delete your `authorized_keys` file for best performance. + +## How to go back to using the `authorized_keys` file + +This is a brief overview. Please refer to the above instructions for more context. + +1. [Rebuild the `authorized_keys` file](../raketasks/maintenance.md#rebuild-authorized_keys-file) +1. Enable writes to the `authorized_keys` file in Application Settings +1. Remove the `AuthorizedKeysCommand` lines from `/etc/ssh/sshd_config` or from `/assets/sshd_config` if you are using Omnibus Docker. +1. Reload sshd: `sudo service sshd reload` +1. Remove the `/opt/gitlab-shell/authorized_keys` file + +## Compiling a custom version of OpenSSH for CentOS 6 + +Building a custom version of OpenSSH is not necessary for Ubuntu 16.04 users, +since Ubuntu 16.04 ships with OpenSSH 7.2. + +It is also unnecessary for CentOS 7.4 users, as that version ships with +OpenSSH 7.4. If you are using CentOS 7.0 - 7.3, we strongly recommend that you +upgrade to CentOS 7.4 instead of following this procedure. This should be as +simple as running `yum update`. + +CentOS 6 users must build their own OpenSSH package to enable SSH lookups via +the database. The following instructions can be used to build OpenSSH 7.5: + +1. First, download the package and install the required packages: + + ``` + sudo su - + cd /tmp + curl --remote-name https://mirrors.evowise.com/pub/OpenBSD/OpenSSH/portable/openssh-7.5p1.tar.gz + tar xzvf openssh-7.5p1.tar.gz + yum install rpm-build gcc make wget openssl-devel krb5-devel pam-devel libX11-devel xmkmf libXt-devel + ``` + +3. Prepare the build by copying files to the right place: + + ``` + mkdir -p /root/rpmbuild/{SOURCES,SPECS} + cp ./openssh-7.5p1/contrib/redhat/openssh.spec /root/rpmbuild/SPECS/ + cp openssh-7.5p1.tar.gz /root/rpmbuild/SOURCES/ + cd /root/rpmbuild/SPECS + ``` + +3. Next, set the spec settings properly: + + ``` + sed -i -e "s/%define no_gnome_askpass 0/%define no_gnome_askpass 1/g" openssh.spec + sed -i -e "s/%define no_x11_askpass 0/%define no_x11_askpass 1/g" openssh.spec + sed -i -e "s/BuildPreReq/BuildRequires/g" openssh.spec + ``` + +3. Build the RPMs: + + ``` + rpmbuild -bb openssh.spec + ``` + +4. Ensure the RPMs were built: + + ``` + ls -al /root/rpmbuild/RPMS/x86_64/ + ``` + + You should see something as the following: + + ``` + total 1324 + drwxr-xr-x. 2 root root 4096 Jun 20 19:37 . + drwxr-xr-x. 3 root root 19 Jun 20 19:37 .. + -rw-r--r--. 1 root root 470828 Jun 20 19:37 openssh-7.5p1-1.x86_64.rpm + -rw-r--r--. 1 root root 490716 Jun 20 19:37 openssh-clients-7.5p1-1.x86_64.rpm + -rw-r--r--. 1 root root 17020 Jun 20 19:37 openssh-debuginfo-7.5p1-1.x86_64.rpm + -rw-r--r--. 1 root root 367516 Jun 20 19:37 openssh-server-7.5p1-1.x86_64.rpm + ``` + +5. Install the packages. OpenSSH packages will replace `/etc/pam.d/sshd` + with its own version, which may prevent users from logging in, so be sure + that the file is backed up and restored after installation: + + ``` + timestamp=$(date +%s) + cp /etc/pam.d/sshd pam-ssh-conf-$timestamp + rpm -Uvh /root/rpmbuild/RPMS/x86_64/*.rpm + yes | cp pam-ssh-conf-$timestamp /etc/pam.d/sshd + ``` + +6. Verify the installed version. In another window, attempt to login to the server: + + ``` + ssh -v <your-centos-machine> + ``` + + You should see a line that reads: "debug1: Remote protocol version 2.0, remote software version OpenSSH_7.5" + + If not, you may need to restart sshd (e.g. `systemctl restart sshd.service`). + +7. *IMPORTANT!* Open a new SSH session to your server before exiting to make + sure everything is working! If you need to downgrade, simple install the + older package: + + ``` + # Only run this if you run into a problem logging in + yum downgrade openssh-server openssh openssh-clients + ``` diff --git a/doc/administration/operations/img/write_to_authorized_keys_setting.png b/doc/administration/operations/img/write_to_authorized_keys_setting.png Binary files differnew file mode 100644 index 00000000000..232765f1917 --- /dev/null +++ b/doc/administration/operations/img/write_to_authorized_keys_setting.png diff --git a/doc/administration/operations/index.md b/doc/administration/operations/index.md index 320d71a9527..5655b7efec6 100644 --- a/doc/administration/operations/index.md +++ b/doc/administration/operations/index.md @@ -13,4 +13,5 @@ by GitLab to another file system or another server. that to prioritize important jobs. - [Sidekiq MemoryKiller](sidekiq_memory_killer.md): Configure Sidekiq MemoryKiller to restart Sidekiq. -- [Unicorn](unicorn.md): Understand Unicorn and unicorn-worker-killer.
\ No newline at end of file +- [Unicorn](unicorn.md): Understand Unicorn and unicorn-worker-killer. +- [Speed up SSH operations](fast_ssh_key_lookup.md): Authorize SSH users via a fast, indexed lookup to the GitLab database. diff --git a/doc/administration/operations/speed_up_ssh.md b/doc/administration/operations/speed_up_ssh.md new file mode 100644 index 00000000000..89265b3018b --- /dev/null +++ b/doc/administration/operations/speed_up_ssh.md @@ -0,0 +1 @@ +This document was moved to [another location](fast_ssh_key_lookup.md). diff --git a/lib/api/internal.rb b/lib/api/internal.rb index 79b302aae70..8bf53939751 100644 --- a/lib/api/internal.rb +++ b/lib/api/internal.rb @@ -82,6 +82,18 @@ module API end # + # Get a ssh key using the fingerprint + # + get "/authorized_keys" do + fingerprint = params.fetch(:fingerprint) do + Gitlab::InsecureKeyFingerprint.new(params.fetch(:key)).fingerprint + end + key = Key.find_by(fingerprint: fingerprint) + not_found!("Key") if key.nil? + present key, with: Entities::SSHKey + end + + # # Discover user by ssh key or user id # get "/discover" do diff --git a/lib/gitlab/insecure_key_fingerprint.rb b/lib/gitlab/insecure_key_fingerprint.rb new file mode 100644 index 00000000000..f85b6e9197f --- /dev/null +++ b/lib/gitlab/insecure_key_fingerprint.rb @@ -0,0 +1,23 @@ +module Gitlab + # + # Calculates the fingerprint of a given key without using + # openssh key validations. For this reason, only use + # for calculating the fingerprint to find the key with it. + # + # DO NOT use it for checking the validity of a ssh key. + # + class InsecureKeyFingerprint + attr_accessor :key + + # + # Gets the base64 encoded string representing a rsa or dsa key + # + def initialize(key_base64) + @key = key_base64 + end + + def fingerprint + OpenSSL::Digest::MD5.hexdigest(Base64.decode64(@key)).scan(/../).join(':') + end + end +end diff --git a/lib/gitlab/shell.rb b/lib/gitlab/shell.rb index a8a4ec996c4..392f66c99d3 100644 --- a/lib/gitlab/shell.rb +++ b/lib/gitlab/shell.rb @@ -183,6 +183,8 @@ module Gitlab # add_key("key-42", "sha-rsa ...") # def add_key(key_id, key_content) + return unless self.authorized_keys_enabled? + gitlab_shell_fast_execute([gitlab_shell_keys_path, 'add-key', key_id, self.class.strip_key(key_content)]) end @@ -192,6 +194,8 @@ module Gitlab # Ex. # batch_add_keys { |adder| adder.add_key("key-42", "sha-rsa ...") } def batch_add_keys(&block) + return unless self.authorized_keys_enabled? + IO.popen(%W(#{gitlab_shell_path}/bin/gitlab-keys batch-add-keys), 'w') do |io| yield(KeyAdder.new(io)) end @@ -202,10 +206,11 @@ module Gitlab # Ex. # remove_key("key-342", "sha-rsa ...") # - def remove_key(key_id, key_content) + def remove_key(key_id, key_content = nil) + return unless self.authorized_keys_enabled? + args = [gitlab_shell_keys_path, 'rm-key', key_id] args << key_content if key_content - gitlab_shell_fast_execute(args) end @@ -215,9 +220,62 @@ module Gitlab # remove_all_keys # def remove_all_keys + return unless self.authorized_keys_enabled? + gitlab_shell_fast_execute([gitlab_shell_keys_path, 'clear']) end + # Remove ssh keys from gitlab shell that are not in the DB + # + # Ex. + # remove_keys_not_found_in_db + # + def remove_keys_not_found_in_db + return unless self.authorized_keys_enabled? + + Rails.logger.info("Removing keys not found in DB") + + 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") + remove_key("key-#{id}") + end + end + end + + # Iterate over all ssh key IDs from gitlab shell, in batches + # + # Ex. + # batch_read_key_ids { |batch| keys = Key.where(id: batch) } + # + def batch_read_key_ids(batch_size: 100, &block) + return unless self.authorized_keys_enabled? + + list_key_ids do |key_id_stream| + key_id_stream.lazy.each_slice(batch_size) do |lines| + key_ids = lines.map { |l| l.chomp.to_i } + yield(key_ids) + end + end + end + + # Stream all ssh key IDs from gitlab shell, separated by newlines + # + # Ex. + # list_key_ids + # + def list_key_ids(&block) + return unless self.authorized_keys_enabled? + + IO.popen(%W(#{gitlab_shell_path}/bin/gitlab-keys list-key-ids), &block) + end + # Add empty directory for storing repositories # # Ex. @@ -333,6 +391,14 @@ module Gitlab File.join(gitlab_shell_path, 'bin', 'gitlab-keys') 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 gitlab_projects(shard_path, disk_path) diff --git a/spec/lib/gitlab/insecure_key_fingerprint_spec.rb b/spec/lib/gitlab/insecure_key_fingerprint_spec.rb new file mode 100644 index 00000000000..6532579b1c9 --- /dev/null +++ b/spec/lib/gitlab/insecure_key_fingerprint_spec.rb @@ -0,0 +1,18 @@ +require 'spec_helper' + +describe Gitlab::InsecureKeyFingerprint do + let(:key) do + 'ssh-rsa AAAAB3NzaC1yc2EAAAABJQAAAIEAiPWx6WM4lhHNedGfBpPJNPpZ7yKu+dnn' \ + '1SJejgt4596k6YjzGGphH2TUxwKzxcKDKKezwkpfnxPkSMkuEspGRt/aZZ9wa++Oi7Qk' \ + 'r8prgHc4soW6NUlfDzpvZK2H5E7eQaSeP3SAwGmQKUFHCddNaP0L+hM7zhFNzjFvpaMg' \ + 'Jw0=' + end + + let(:fingerprint) { "3f:a2:ee:de:b5:de:53:c3:aa:2f:9c:45:24:4c:47:7b" } + + describe "#fingerprint" do + it "generates the key's fingerprint" do + expect(described_class.new(key.split[1]).fingerprint).to eq(fingerprint) + end + end +end diff --git a/spec/lib/gitlab/shell_spec.rb b/spec/lib/gitlab/shell_spec.rb index ffd2d2c7afc..aed4855906e 100644 --- a/spec/lib/gitlab/shell_spec.rb +++ b/spec/lib/gitlab/shell_spec.rb @@ -52,6 +52,311 @@ describe Gitlab::Shell do end end + describe '#add_key' do + context 'when authorized_keys_enabled is true' do + it 'removes trailing garbage' do + allow(gitlab_shell).to receive(:gitlab_shell_keys_path).and_return(:gitlab_shell_keys_path) + expect(gitlab_shell).to receive(:gitlab_shell_fast_execute).with( + [:gitlab_shell_keys_path, 'add-key', 'key-123', 'ssh-rsa foobar'] + ) + + gitlab_shell.add_key('key-123', 'ssh-rsa foobar trailing garbage') + 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_shell).not_to receive(:gitlab_shell_fast_execute) + + 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 'removes trailing garbage' do + allow(gitlab_shell).to receive(:gitlab_shell_keys_path).and_return(:gitlab_shell_keys_path) + expect(gitlab_shell).to receive(:gitlab_shell_fast_execute).with( + [:gitlab_shell_keys_path, 'add-key', 'key-123', 'ssh-rsa foobar'] + ) + + gitlab_shell.add_key('key-123', 'ssh-rsa foobar trailing garbage') + end + end + end + + describe '#batch_add_keys' do + context 'when authorized_keys_enabled is true' do + it 'instantiates KeyAdder' do + expect_any_instance_of(Gitlab::Shell::KeyAdder).to receive(:add_key).with('key-123', 'ssh-rsa foobar') + + gitlab_shell.batch_add_keys do |adder| + adder.add_key('key-123', 'ssh-rsa foobar') + end + 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_any_instance_of(Gitlab::Shell::KeyAdder).not_to receive(:add_key) + + gitlab_shell.batch_add_keys do |adder| + adder.add_key('key-123', 'ssh-rsa foobar') + end + end + end + + context 'when authorized_keys_enabled is nil' do + before do + stub_application_setting(authorized_keys_enabled: nil) + end + + it 'instantiates KeyAdder' do + expect_any_instance_of(Gitlab::Shell::KeyAdder).to receive(:add_key).with('key-123', 'ssh-rsa foobar') + + gitlab_shell.batch_add_keys do |adder| + adder.add_key('key-123', 'ssh-rsa foobar') + end + end + end + end + + describe '#remove_key' do + context 'when authorized_keys_enabled is true' do + it 'removes trailing garbage' do + allow(gitlab_shell).to receive(:gitlab_shell_keys_path).and_return(:gitlab_shell_keys_path) + expect(gitlab_shell).to receive(:gitlab_shell_fast_execute).with( + [:gitlab_shell_keys_path, 'rm-key', 'key-123', 'ssh-rsa foobar'] + ) + + gitlab_shell.remove_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_shell).not_to receive(:gitlab_shell_fast_execute) + + gitlab_shell.remove_key('key-123', 'ssh-rsa foobar') + end + end + + context 'when authorized_keys_enabled is nil' do + before do + stub_application_setting(authorized_keys_enabled: nil) + end + + it 'removes trailing garbage' do + allow(gitlab_shell).to receive(:gitlab_shell_keys_path).and_return(:gitlab_shell_keys_path) + expect(gitlab_shell).to receive(:gitlab_shell_fast_execute).with( + [:gitlab_shell_keys_path, 'rm-key', 'key-123', 'ssh-rsa foobar'] + ) + + gitlab_shell.remove_key('key-123', 'ssh-rsa foobar') + end + end + + context 'when key content is not given' do + it 'calls rm-key with only one argument' do + allow(gitlab_shell).to receive(:gitlab_shell_keys_path).and_return(:gitlab_shell_keys_path) + expect(gitlab_shell).to receive(:gitlab_shell_fast_execute).with( + [:gitlab_shell_keys_path, 'rm-key', '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 'removes trailing garbage' do + allow(gitlab_shell).to receive(:gitlab_shell_keys_path).and_return(:gitlab_shell_keys_path) + expect(gitlab_shell).to receive(:gitlab_shell_fast_execute).with([:gitlab_shell_keys_path, '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_shell).not_to receive(:gitlab_shell_fast_execute) + + 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 'removes trailing garbage' do + allow(gitlab_shell).to receive(:gitlab_shell_keys_path).and_return(:gitlab_shell_keys_path) + expect(gitlab_shell).to receive(:gitlab_shell_fast_execute).with( + [:gitlab_shell_keys_path, '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(find_in_authorized_keys_file(1234)).to be_truthy + expect(find_in_authorized_keys_file(9876)).to be_truthy + expect(find_in_authorized_keys_file(@another_key.id)).to be_truthy + gitlab_shell.remove_keys_not_found_in_db + expect(find_in_authorized_keys_file(1234)).to be_falsey + expect(find_in_authorized_keys_file(9876)).to be_falsey + expect(find_in_authorized_keys_file(@another_key.id)).to be_truthy + 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(find_in_authorized_keys_file(1234)).to be_truthy + gitlab_shell.remove_keys_not_found_in_db + expect(find_in_authorized_keys_file(1234)).to be_falsey + end + + it 'does not run remove more than once per key (in a batch)' do + expect(gitlab_shell).to receive(:remove_key).with('key-1234').once + 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 + gitlab_shell.remove_keys_not_found_in_db + expect(find_in_authorized_keys_file(@key.id)).to be_truthy + end + + it 'does not need to run a SELECT query for that batch, on account of that key' do + expect_any_instance_of(ActiveRecord::Relation).not_to receive(:pluck) + 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(find_in_authorized_keys_file(1234)).to be_truthy + gitlab_shell.remove_keys_not_found_in_db + expect(find_in_authorized_keys_file(1234)).to be_falsey + end + end + end + end + + describe '#batch_read_key_ids' do + context 'when there are keys in the authorized_keys file' do + before do + gitlab_shell.remove_all_keys + (1..4).each do |i| + gitlab_shell.add_key("key-#{i}", "ssh-rsa ASDFASDF#{i}") + end + end + + it 'iterates over the key IDs in the file, in batches' do + loop_count = 0 + first_batch = [1, 2] + second_batch = [3, 4] + + gitlab_shell.batch_read_key_ids(batch_size: 2) do |batch| + expected = (loop_count == 0 ? first_batch : second_batch) + expect(batch).to eq(expected) + loop_count += 1 + end + end + end + end + + describe '#list_key_ids' do + context 'when there are keys in the authorized_keys file' do + before do + gitlab_shell.remove_all_keys + (1..4).each do |i| + gitlab_shell.add_key("key-#{i}", "ssh-rsa ASDFASDF#{i}") + end + end + + it 'outputs the key IDs in the file, separated by newlines' do + ids = [] + gitlab_shell.list_key_ids do |io| + io.each do |line| + ids << line + end + end + + expect(ids).to eq(%W{1\n 2\n 3\n 4\n}) + end + end + + context 'when there are no keys in the authorized_keys file' do + before do + gitlab_shell.remove_all_keys + end + + it 'outputs nothing, not even an empty string' do + ids = [] + gitlab_shell.list_key_ids do |io| + io.each do |line| + ids << line + end + end + + expect(ids).to eq([]) + end + end + end + describe Gitlab::Shell::KeyAdder do describe '#add_key' do it 'removes trailing garbage' do @@ -97,17 +402,6 @@ describe Gitlab::Shell do allow(Gitlab.config.gitlab_shell).to receive(:git_timeout).and_return(800) end - describe '#add_key' do - it 'removes trailing garbage' do - allow(gitlab_shell).to receive(:gitlab_shell_keys_path).and_return(:gitlab_shell_keys_path) - expect(gitlab_shell).to receive(:gitlab_shell_fast_execute).with( - [:gitlab_shell_keys_path, 'add-key', 'key-123', 'ssh-rsa foobar'] - ) - - gitlab_shell.add_key('key-123', 'ssh-rsa foobar trailing garbage') - end - end - describe '#add_repository' do shared_examples '#add_repository' do let(:repository_storage) { 'default' } @@ -412,4 +706,12 @@ describe Gitlab::Shell do end end end + + def find_in_authorized_keys_file(key_id) + gitlab_shell.batch_read_key_ids do |ids| + return true if ids.include?(key_id) + end + + false + end end diff --git a/spec/requests/api/internal_spec.rb b/spec/requests/api/internal_spec.rb index 7b25047ea8f..2783c51b8df 100644 --- a/spec/requests/api/internal_spec.rb +++ b/spec/requests/api/internal_spec.rb @@ -192,6 +192,54 @@ describe API::Internal do end end + describe "GET /internal/authorized_keys" do + context "using an existing key's fingerprint" do + it "finds the key" do + get(api('/internal/authorized_keys'), fingerprint: key.fingerprint, secret_token: secret_token) + + expect(response.status).to eq(200) + expect(json_response["key"]).to eq(key.key) + end + end + + context "non existing key's fingerprint" do + it "returns 404" do + get(api('/internal/authorized_keys'), fingerprint: "no:t-:va:li:d0", secret_token: secret_token) + + expect(response.status).to eq(404) + end + end + + context "using a partial fingerprint" do + it "returns 404" do + get(api('/internal/authorized_keys'), fingerprint: "#{key.fingerprint[0..5]}%", secret_token: secret_token) + + expect(response.status).to eq(404) + end + end + + context "sending the key" do + it "finds the key" do + get(api('/internal/authorized_keys'), key: key.key.split[1], secret_token: secret_token) + + expect(response.status).to eq(200) + expect(json_response["key"]).to eq(key.key) + end + + it "returns 404 with a partial key" do + get(api('/internal/authorized_keys'), key: key.key.split[1][0...-3], secret_token: secret_token) + + expect(response.status).to eq(404) + end + + it "returns 404 with an not valid base64 string" do + get(api('/internal/authorized_keys'), key: "whatever!", secret_token: secret_token) + + expect(response.status).to eq(404) + end + end + end + describe "POST /internal/allowed", :clean_gitlab_redis_shared_state do context "access granted" do around do |example| diff --git a/spec/workers/gitlab_shell_worker_spec.rb b/spec/workers/gitlab_shell_worker_spec.rb new file mode 100644 index 00000000000..6b222af454d --- /dev/null +++ b/spec/workers/gitlab_shell_worker_spec.rb @@ -0,0 +1,12 @@ +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_any_instance_of(Gitlab::Shell).to receive(:add_key).with('foo', 'bar') + worker.perform(:add_key, 'foo', 'bar') + end + end +end |