summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorNick Thomas <nick@gitlab.com>2017-08-30 12:00:39 +0100
committerNick Thomas <nick@gitlab.com>2017-09-04 18:16:31 +0100
commit48115be509ce00120d0609f5f18a5bc3804bb21f (patch)
tree6ead152e6608e1c3d30de6469efc4b05dd090e0b
parent25a443d65220cb76fab2c8123eca17f30c461a89 (diff)
downloadgitlab-ce-48115be509ce00120d0609f5f18a5bc3804bb21f.tar.gz
Add a system check for the git user's custom SSH configuration
-rw-r--r--changelogs/unreleased/37204-deprecate-git-user-manual-ssh-config.yml5
-rw-r--r--doc/ssh/README.md32
-rw-r--r--lib/system_check/app/git_user_default_ssh_config_check.rb69
-rw-r--r--lib/tasks/gitlab/check.rake1
-rw-r--r--spec/lib/system_check/app/git_user_default_ssh_config_check_spec.rb79
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