diff options
| -rw-r--r-- | changelogs/unreleased/37204-deprecate-git-user-manual-ssh-config.yml | 5 | ||||
| -rw-r--r-- | doc/ssh/README.md | 32 | ||||
| -rw-r--r-- | lib/system_check/app/git_user_default_ssh_config_check.rb | 69 | ||||
| -rw-r--r-- | lib/tasks/gitlab/check.rake | 1 | ||||
| -rw-r--r-- | spec/lib/system_check/app/git_user_default_ssh_config_check_spec.rb | 79 |
5 files changed, 186 insertions, 0 deletions
diff --git a/changelogs/unreleased/37204-deprecate-git-user-manual-ssh-config.yml b/changelogs/unreleased/37204-deprecate-git-user-manual-ssh-config.yml new file mode 100644 index 00000000000..593e74593c4 --- /dev/null +++ b/changelogs/unreleased/37204-deprecate-git-user-manual-ssh-config.yml @@ -0,0 +1,5 @@ +--- +title: Deprecate custom SSH client configuration for the git user +merge_request: 13930 +author: +type: deprecated diff --git a/doc/ssh/README.md b/doc/ssh/README.md index cf28f1a2eca..793de9d777c 100644 --- a/doc/ssh/README.md +++ b/doc/ssh/README.md @@ -193,6 +193,38 @@ How to add your SSH key to Eclipse: https://wiki.eclipse.org/EGit/User_Guide#Ecl [winputty]: https://the.earth.li/~sgtatham/putty/0.67/htmldoc/Chapter8.html#pubkey-puttygen +## SSH on the GitLab server + +GitLab integrates with the system-installed SSH daemon, designating a user +(typically named `git`) through which all access requests are handled. Users +connecting to the GitLab server over SSH are identified by their SSH key instead +of their username. + +SSH *client* operations performed on the GitLab server wil be executed as this +user. Although it is possible to modify the SSH configuration for this user to, +e.g., provide a private SSH key to authenticate these requests by, this practice +is **not supported** and is strongly discouraged as it presents significant +security risks. + +The GitLab check process includes a check for this condition, and will direct you +to this section if your server is configured like this, e.g.: + +``` +$ gitlab-rake gitlab:check +# ... +Git user has default SSH configuration? ... no + Try fixing it: + mkdir ~/gitlab-check-backup-1504540051 + sudo mv /var/lib/git/.ssh/id_rsa ~/gitlab-check-backup-1504540051 + sudo mv /var/lib/git/.ssh/id_rsa.pub ~/gitlab-check-backup-1504540051 + For more information see: + doc/ssh/README.md in section "SSH on the GitLab server" + Please fix the error above and rerun the checks. +``` + +Remove the custom configuration as soon as you're able to. These customizations +are *explicitly not supported* and may stop working at any time. + ## Troubleshooting If on Git clone you are prompted for a password like `git@gitlab.com's password:` diff --git a/lib/system_check/app/git_user_default_ssh_config_check.rb b/lib/system_check/app/git_user_default_ssh_config_check.rb new file mode 100644 index 00000000000..7b486d78cf0 --- /dev/null +++ b/lib/system_check/app/git_user_default_ssh_config_check.rb @@ -0,0 +1,69 @@ +module SystemCheck + module App + class GitUserDefaultSSHConfigCheck < SystemCheck::BaseCheck + # These files are allowed in the .ssh directory. The `config` file is not + # whitelisted as it may change the SSH client's behaviour dramatically. + WHITELIST = %w[ + authorized_keys + authorized_keys2 + known_hosts + ].freeze + + set_name 'Git user has default SSH configuration?' + set_skip_reason 'skipped (git user is not present or configured)' + + def skip? + !home_dir || !File.directory?(home_dir) + end + + def check? + forbidden_files.empty? + end + + def show_error + backup_dir = "~/gitlab-check-backup-#{Time.now.to_i}" + + instructions = forbidden_files.map do |filename| + "sudo mv #{Shellwords.escape(filename)} #{backup_dir}" + end + + try_fixing_it("mkdir #{backup_dir}", *instructions) + for_more_information('doc/ssh/README.md in section "SSH on the GitLab server"') + fix_and_rerun + end + + private + + def git_user + Gitlab.config.gitlab.user + end + + def home_dir + return @home_dir if defined?(@home_dir) + + @home_dir = + begin + File.expand_path("~#{git_user}") + rescue ArgumentError + nil + end + end + + def ssh_dir + return nil unless home_dir + + File.join(home_dir, '.ssh') + end + + def forbidden_files + @forbidden_files ||= + begin + present = Dir[File.join(ssh_dir, '*')] + whitelisted = WHITELIST.map { |basename| File.join(ssh_dir, basename) } + + present - whitelisted + end + end + end + end +end diff --git a/lib/tasks/gitlab/check.rake b/lib/tasks/gitlab/check.rake index 1bd36bbe20a..92a3f503fcb 100644 --- a/lib/tasks/gitlab/check.rake +++ b/lib/tasks/gitlab/check.rake @@ -33,6 +33,7 @@ namespace :gitlab do SystemCheck::App::RedisVersionCheck, SystemCheck::App::RubyVersionCheck, SystemCheck::App::GitVersionCheck, + SystemCheck::App::GitUserDefaultSSHConfigCheck, SystemCheck::App::ActiveUsersCheck ] diff --git a/spec/lib/system_check/app/git_user_default_ssh_config_check_spec.rb b/spec/lib/system_check/app/git_user_default_ssh_config_check_spec.rb new file mode 100644 index 00000000000..7125bfcab59 --- /dev/null +++ b/spec/lib/system_check/app/git_user_default_ssh_config_check_spec.rb @@ -0,0 +1,79 @@ +require 'spec_helper' + +describe SystemCheck::App::GitUserDefaultSSHConfigCheck do + let(:username) { '_this_user_will_not_exist_unless_it_is_stubbed' } + let(:base_dir) { Dir.mktmpdir } + let(:home_dir) { File.join(base_dir, "/var/lib/#{username}") } + let(:ssh_dir) { File.join(home_dir, '.ssh') } + let(:forbidden_file) { 'id_rsa' } + + before do + allow(Gitlab.config.gitlab).to receive(:user).and_return(username) + end + + after do + FileUtils.rm_rf(base_dir) + end + + it 'only whitelists safe files' do + expect(described_class::WHITELIST).to contain_exactly('authorized_keys', 'authorized_keys2', 'known_hosts') + end + + describe '#skip?' do + subject { described_class.new.skip? } + + where(user_exists: [true, false], home_dir_exists: [true, false]) + + with_them do + let(:expected_result) { !user_exists || !home_dir_exists } + + before do + stub_user if user_exists + stub_home_dir if home_dir_exists + end + + it { is_expected.to eq(expected_result) } + end + end + + describe '#check?' do + subject { described_class.new.check? } + + before do + stub_user + end + + it 'fails if a forbidden file exists' do + stub_ssh_file(forbidden_file) + + is_expected.to be_falsy + end + + it "succeeds if the SSH directory doesn't exist" do + FileUtils.rm_rf(ssh_dir) + + is_expected.to be_truthy + end + + it 'succeeds if all the whitelisted files exist' do + described_class::WHITELIST.each do |filename| + stub_ssh_file(filename) + end + + is_expected.to be_truthy + end + end + + def stub_user + allow(File).to receive(:expand_path).with("~#{username}").and_return(home_dir) + end + + def stub_home_dir + FileUtils.mkdir_p(home_dir) + end + + def stub_ssh_file(filename) + FileUtils.mkdir_p(ssh_dir) + FileUtils.touch(File.join(ssh_dir, filename)) + end +end |