Fix two regressions in SSH certificate support

Fix two regressions in my 2e8b6702 ("Add support for SSH certificate
authentication", 2018-06-14) merged in gitlab-org/gitlab-shell!207.

This fixes the issue noted in gitlab-org/gitlab-shell#145 where the
command-line contains things other than the key/user/username, and
also a regression where SSH certificates are being used, and the
username presented in the key is unknown to GitLab.

In that case, we should log the user in as "Anonymous" (on an instance
that allows public access), but because of how the error checking
around api.discover() was implemented we ended up erroring out
instead.
parent c6577e0d
...@@ -17,7 +17,11 @@ require_relative '../lib/gitlab_init' ...@@ -17,7 +17,11 @@ require_relative '../lib/gitlab_init'
# #
require File.join(ROOT_PATH, 'lib', 'gitlab_shell') require File.join(ROOT_PATH, 'lib', 'gitlab_shell')
if GitlabShell.new(ARGV.join).exec(original_cmd) # We must match e.g. "key-12345" anywhere on the command-line. See
# https://gitlab.com/gitlab-org/gitlab-shell/issues/145
who = /\b(?:(?:key|user)-[0-9]+|username-\S+)\b/.match(ARGV.join).to_s;
if GitlabShell.new(who).exec(original_cmd)
exit 0 exit 0
else else
exit 1 exit 1
......
...@@ -208,7 +208,7 @@ class GitlabShell # rubocop:disable Metrics/ClassLength ...@@ -208,7 +208,7 @@ class GitlabShell # rubocop:disable Metrics/ClassLength
begin begin
if defined?(@who) if defined?(@who)
@user = api.discover(@who) @user = api.discover(@who)
@gl_id = "user-#{@user['id']}" @gl_id = "user-#{@user['id']}" if @user
else else
@user = api.discover(@gl_id) @user = api.discover(@gl_id)
end end
......
...@@ -5,6 +5,9 @@ describe 'bin/gitlab-shell-authorized-keys-check' do ...@@ -5,6 +5,9 @@ describe 'bin/gitlab-shell-authorized-keys-check' do
ROOT_PATH ROOT_PATH
end end
# All this test boilerplate is mostly copy/pasted between
# gitlab_shell_gitlab_shell_spec.rb and
# gitlab_shell_authorized_keys_check_spec.rb
def tmp_root_path def tmp_root_path
@tmp_root_path ||= File.realpath(Dir.mktmpdir) @tmp_root_path ||= File.realpath(Dir.mktmpdir)
end end
......
require_relative 'spec_helper'
describe 'bin/gitlab-shell' do
def original_root_path
ROOT_PATH
end
# All this test boilerplate is mostly copy/pasted between
# gitlab_shell_gitlab_shell_spec.rb and
# gitlab_shell_authorized_keys_check_spec.rb
def tmp_root_path
@tmp_root_path ||= File.realpath(Dir.mktmpdir)
end
def config_path
File.join(tmp_root_path, 'config.yml')
end
def tmp_socket_path
# This has to be a relative path shorter than 100 bytes due to
# limitations in how Unix sockets work.
'tmp/gitlab-shell-socket'
end
before(:all) do
FileUtils.mkdir_p(File.dirname(tmp_socket_path))
FileUtils.touch(File.join(tmp_root_path, '.gitlab_shell_secret'))
@server = HTTPUNIXServer.new(BindAddress: tmp_socket_path)
@server.mount_proc('/api/v4/internal/discover') do |req, res|
if req.query['key_id'] == '100' ||
req.query['user_id'] == '10' ||
req.query['username'] == 'someuser'
res.status = 200
res.content_type = 'application/json'
res.body = '{"id":1, "name": "Some User", "username": "someuser"}'
else
res.status = 500
end
end
@webrick_thread = Thread.new { @server.start }
sleep(0.1) while @webrick_thread.alive? && @server.status != :Running
raise "Couldn't start stub GitlabNet server" unless @server.status == :Running
File.open(config_path, 'w') do |f|
f.write("---\ngitlab_url: http+unix://#{CGI.escape(tmp_socket_path)}\n")
end
copy_dirs = ['bin', 'lib']
FileUtils.rm_rf(copy_dirs.map { |d| File.join(tmp_root_path, d) })
FileUtils.cp_r(copy_dirs, tmp_root_path)
end
after(:all) do
@server.shutdown if @server
@webrick_thread.join if @webrick_thread
FileUtils.rm_rf(tmp_root_path)
end
let(:gitlab_shell_path) { File.join(tmp_root_path, 'bin', 'gitlab-shell') }
# Basic valid input
it 'succeeds and prints username when a valid known key id is given' do
output, status = run!(["key-100"])
expect(output).to eq("Welcome to GitLab, @someuser!\n")
expect(status).to be_success
end
it 'succeeds and prints username when a valid known user id is given' do
output, status = run!(["user-10"])
expect(output).to eq("Welcome to GitLab, @someuser!\n")
expect(status).to be_success
end
it 'succeeds and prints username when a valid known username is given' do
output, status = run!(["username-someuser"])
expect(output).to eq("Welcome to GitLab, @someuser!\n")
expect(status).to be_success
end
# Valid but unknown input
it 'succeeds and prints Anonymous when a valid unknown key id is given' do
output, status = run!(["key-12345"])
expect(output).to eq("Welcome to GitLab, Anonymous!\n")
expect(status).to be_success
end
it 'succeeds and prints Anonymous when a valid unknown user id is given' do
output, status = run!(["user-12345"])
expect(output).to eq("Welcome to GitLab, Anonymous!\n")
expect(status).to be_success
end
it 'succeeds and prints Anonymous when a valid unknown username is given' do
output, status = run!(["username-unknown"])
expect(output).to eq("Welcome to GitLab, Anonymous!\n")
expect(status).to be_success
end
# Invalid input. TODO: capture stderr & compare
it 'gets an ArgumentError on invalid input (empty)' do
output, status = run!([])
expect(output).to eq("")
expect(status).not_to be_success
end
it 'gets an ArgumentError on invalid input (unknown)' do
output, status = run!(["whatever"])
expect(output).to eq("")
expect(status).not_to be_success
end
it 'gets an ArgumentError on invalid input (multiple unknown)' do
output, status = run!(["this", "is", "all", "invalid"])
expect(output).to eq("")
expect(status).not_to be_success
end
# Not so basic valid input
# (https://gitlab.com/gitlab-org/gitlab-shell/issues/145)
it 'succeeds and prints username when a valid known key id is given in the middle of other input' do
output, status = run!(["-c/usr/share/webapps/gitlab-shell/bin/gitlab-shell key-100"])
expect(output).to eq("Welcome to GitLab, @someuser!\n")
expect(status).to be_success
end
it 'succeeds and prints username when a valid known user id is given in the middle of other input' do
output, status = run!(["-c/usr/share/webapps/gitlab-shell/bin/gitlab-shell user-10"])
expect(output).to eq("Welcome to GitLab, @someuser!\n")
expect(status).to be_success
end
it 'succeeds and prints username when a valid known username is given in the middle of other input' do
output, status = run!(["-c/usr/share/webapps/gitlab-shell/bin/gitlab-shell username-someuser"])
expect(output).to eq("Welcome to GitLab, @someuser!\n")
expect(status).to be_success
end
def run!(args)
cmd = [
gitlab_shell_path,
args
].flatten.compact
output = IO.popen({'SSH_CONNECTION' => 'fake'}, cmd, &:read)
[output, $?]
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