Merge branch 'release/undev_v2.2.0'

* release/undev_v2.2.0: (29 commits)
  Version 1.8.0
  Stub file writing in tests
  knife-undev as loader chef cookbooks
  fixes knife in hooks
  Removing extra space from the end of gitlab_update error messages
  Redis configuration check
  Display error and send failure exit status if redis-cli fails
  Display appropriate error message on config error
  Fix return values in GitlabKeys
  Refactor file writing tests
  1.7.9
  Fix relative path detection for ssh://host:port/repo.git
  Remove unused open3 dependency
  Allow bin/install to signal failure on exit
  Add CVEs to CHANGELOG
  Version 1.7.8
  Escape repository path
  Check for redis-cli binary
  Update .gitignore
  Bypass shell and use stdlib JSON for GitlabUpdate
  ...

.gitignore

@@ -1,3 +1,4 @@
 config.yml
 tmp/*
 *.log
+/*.log.*

CHANGELOG

@@ -1,8 +1,27 @@
+v1.8.0
+  - Fix return values in GitlabKeys
+
+v1.7.9
+  - Fix escape of repository path for custom ssh port
+
+v1.7.8
+  - Escape repository path to prevent relative links (CVE-2013-4583)
+
+v1.7.7
+  - Separate options from arguments with -- (CVE-2013-4582)
+  - Bypass shell and use stdlib JSON for GitlabUpdate (CVE-2013-4581)
+
+v1.7.6
+  - Fix gitlab-projects update-head for improted repo when branch exists but not listed in refs/head
+
+v1.7.5
+  - Remove keys from authorized_keys using ruby instead of shell
+
 v1.7.4
-  - More protection against shell injection
+  - More protection against shell injection (CVE-2013-4546)
 
 v1.7.3
-  - Use Kernel#open to append lines to authorized_keys
+  - Use Kernel#open to append lines to authorized_keys (CVE-2013-4490)
 
 v1.7.2
   - More safe command execution

VERSION

@@ -1 +1 @@
-1.7.4
+1.8.0

bin/check

@@ -12,7 +12,7 @@ resp = GitlabNet.new.check
 if resp.code == "200"
   print 'OK'
 else
-  puts "FAILED. code: #{resp.code}"
+  abort "FAILED. code: #{resp.code}"
 end
 
 puts "\nCheck directories and files: "
@@ -21,11 +21,18 @@ config = GitlabConfig.new
 dirs = [config.repos_path, config.auth_file]
 
 dirs.each do |dir|
+  abort("ERROR: missing option in config.yml") unless dir
   print "\t#{dir}: "
   if File.exists?(dir)
     print 'OK'
   else
-    puts "FAILED"
+    abort "FAILED"
   end
   puts "\n"
 end
+
+print "Test redis-cli executable: "
+abort('FAILED') unless system(*config.redis_command, '--version')
+
+print "Send ping to redis server: "
+abort unless system(*config.redis_command, 'ping')

bin/install

@@ -20,7 +20,13 @@ commands = [
 ]
 
 commands.each do |cmd|
-  puts "#{cmd}: #{system(cmd)}"
+  print "#{cmd}: "
+  if system(cmd)
+    puts 'OK'
+  else
+    puts 'Failed'
+    abort "#{$PROGRAM_NAME} failed"
+  end
 end
 
 exit

lib/gitlab_config.rb

@@ -52,12 +52,12 @@ def redis_command
     if redis.empty?
       # Default to old method of connecting to redis
       # for users that haven't updated their configuration
-      "env -i redis-cli"
+      %W(env -i redis-cli)
     else
       if redis.has_key?("socket")
-        "#{redis['bin']} -s #{redis['socket']}"
+        %W(#{redis['bin']} -s #{redis['socket']})
       else
-        "#{redis['bin']} -h #{redis['host']} -p #{redis['port']}"
+        %W(#{redis['bin']} -h #{redis['host']} -p #{redis['port']})
       end
     end
   end

lib/gitlab_keys.rb

@@ -31,19 +31,27 @@ def exec
 
   def add_key
     $logger.info "Adding key #{@key_id} => #{@key.inspect}"
-    cmd = "command=\"#{@config.gitlab_shell_path}/bin/gitlab-shell #{@key_id}\",no-port-forwarding,no-X11-forwarding,no-agent-forwarding,no-pty #{@key}"
-    open(auth_file, 'a') { |file| file.puts(cmd) }
+    auth_line = "command=\"#{@config.gitlab_shell_path}/bin/gitlab-shell #{@key_id}\",no-port-forwarding,no-X11-forwarding,no-agent-forwarding,no-pty #{@key}"
+    open(auth_file, 'a') { |file| file.puts(auth_line) }
+    true
   end
 
   def rm_key
     $logger.info "Removing key #{@key_id}"
     Tempfile.open('authorized_keys') do |temp|
-      cmd = "sed '/shell #{@key_id}\"/d' #{auth_file} > #{temp.path} && mv #{temp.path} #{auth_file}"
-      system(cmd)
+      open(auth_file, 'r+') do |current|
+        current.each do |line|
+          temp.puts(line) unless line.include?("/bin/gitlab-shell #{@key_id}\"")
+        end
+      end
+      temp.close
+      FileUtils.cp(temp.path, auth_file)
     end
+    true
   end
 
   def clear
-    system("echo '# Managed by gitlab-shell' > #{auth_file}")
+    open(auth_file, 'w') { |file| file.puts '# Managed by gitlab-shell' }
+    true
   end
 end

lib/gitlab_projects.rb

@@ -1,4 +1,3 @@
-require 'open3'
 require 'fileutils'
 
 require_relative 'gitlab_config'
@@ -51,7 +50,7 @@ def exec
   def create_branch
     branch_name = ARGV.shift
     ref = ARGV.shift || "HEAD"
-    cmd = %W(git --git-dir=#{full_path} branch #{branch_name} #{ref})
+    cmd = %W(git --git-dir=#{full_path} branch -- #{branch_name} #{ref})
     system(*cmd)
   end
 
@@ -64,7 +63,7 @@ def rm_branch
   def create_tag
     tag_name = ARGV.shift
     ref = ARGV.shift || "HEAD"
-    cmd = %W(git --git-dir=#{full_path} tag #{tag_name} #{ref})
+    cmd = %W(git --git-dir=#{full_path} tag -- #{tag_name} #{ref})
     system(*cmd)
   end
 
@@ -98,8 +97,8 @@ def rm_project
   def import_project
     @source = ARGV.shift
     $logger.info "Importing project #{@project_name} from <#{@source}> to <#{full_path}>."
-    cmd = %W(git clone --bare #{@source} #{project_name})
-    system(*cmd, chdir: repos_path) && create_hooks(full_path)
+    cmd = %W(git clone --bare -- #{@source} #{full_path})
+    system(*cmd) && create_hooks(full_path)
   end
 
   # Move repository from one directory to another
@@ -174,8 +173,8 @@ def fork_project
     end
 
     $logger.info "Forking project from <#{full_path}> to <#{full_destination_path}>."
-    cmd = %W(git clone --bare #{full_path})
-    system(*cmd, chdir: namespaced_path) && create_hooks(full_destination_path)
+    cmd = %W(git clone --bare -- #{full_path} #{full_destination_path})
+    system(*cmd) && create_hooks(full_destination_path)
   end
 
   def update_head
@@ -186,11 +185,6 @@ def update_head
       return false
     end
 
-    unless File.exists?(File.join(full_path, 'refs/heads', new_head))
-      $logger.error "update-head failed: specified branch does not exist in ref/heads."
-      return false
-    end
-
     File.open(File.join(full_path, 'HEAD'), 'w') do |f|
       f.write("ref: refs/heads/#{new_head}")
     end

lib/gitlab_shell.rb

@@ -1,4 +1,3 @@
-require 'open3'
 require 'shellwords'
 
 require_relative 'gitlab_net'
@@ -43,7 +42,7 @@ def exec
   def parse_cmd
     args = Shellwords.shellwords(@origin_cmd)
     @git_cmd = args[0]
-    @repo_name = args[1]
+    @repo_name = escape_path(args[1])
   end
 
   def git_cmds
@@ -95,4 +94,14 @@ def username
   def log_username
     @config.audit_usernames ? username : "user with key #{@key_id}"
   end
+
+  def escape_path(path)
+    full_repo_path = File.join(repos_path, path)
+
+    if File.absolute_path(full_repo_path) == full_repo_path
+      path
+    else
+      raise "Wrong repository path"
+    end
+  end
 end

lib/gitlab_update.rb

@@ -1,5 +1,6 @@
 require_relative 'gitlab_init'
 require_relative 'gitlab_net'
+require 'json'
 
 class GitlabUpdate
   attr_reader :config
@@ -33,7 +34,7 @@ def exec
         update_redis
         exit 0
       else
-        puts "GitLab: You are not allowed to access #{@branch_name}! "
+        puts "GitLab: You are not allowed to access #{@branch_name}!"
         exit 1
       end
     else
@@ -53,7 +54,11 @@ def ssh?
   end
 
   def update_redis
-    command = "#{config.redis_command} rpush '#{config.redis_namespace}:queue:post_receive' '{\"class\":\"PostReceive\",\"args\":[\"#{@repo_path}\",\"#{@oldrev}\",\"#{@newrev}\",\"#{@refname}\",\"#{@key_id}\"]}' > /dev/null 2>&1"
-    system(command)
+    queue = "#{config.redis_namespace}:queue:post_receive"
+    msg = JSON.dump({'class' => 'PostReceive', 'args' => [@repo_path, @oldrev, @newrev, @refname, @key_id]})
+    unless system(*config.redis_command, 'rpush', queue, msg, err: '/dev/null', out: '/dev/null')
+      puts "GitLab: An unexpected error occurred (redis-cli returned #{$?.exitstatus})."
+      exit 1
+    end
   end
 end

spec/gitlab_keys_spec.rb

@@ -14,39 +14,65 @@
     it { gitlab_keys.instance_variable_get(:@key_id).should == 'key-741' }
   end
 
+
   describe :add_key do
     let(:gitlab_keys) { build_gitlab_keys('add-key', 'key-741', 'ssh-rsa AAAAB3NzaDAxx2E') }
-    let(:file) { mock(:file) }
 
-    it "should receive valid cmd" do
-      auth_line = "command=\"#{GitlabConfig.new.gitlab_shell_path}/bin/gitlab-shell key-741\",no-port-forwarding,no-X11-forwarding,no-agent-forwarding,no-pty ssh-rsa AAAAB3NzaDAxx2E"
-      gitlab_keys.should_receive(:open).with(GitlabConfig.new.auth_file, 'a').and_yield(file)
-      file.should_receive(:puts).with(auth_line)
+    it "adds a line at the end of the file" do
+      create_authorized_keys_fixture
       gitlab_keys.send :add_key
+      auth_line = "command=\"#{GitlabConfig.new.gitlab_shell_path}/bin/gitlab-shell key-741\",no-port-forwarding,no-X11-forwarding,no-agent-forwarding,no-pty ssh-rsa AAAAB3NzaDAxx2E"
+      File.read(tmp_authorized_keys_path).should == "existing content\n#{auth_line}\n"
     end
 
-    it "should log an add-key event" do
-      $logger.should_receive(:info).with('Adding key key-741 => "ssh-rsa AAAAB3NzaDAxx2E"')
-      gitlab_keys.stub(:open)
-      gitlab_keys.send :add_key
+    context "without file writing" do
+      before { gitlab_keys.stub(:open) }
+
+      it "should log an add-key event" do
+        $logger.should_receive(:info).with('Adding key key-741 => "ssh-rsa AAAAB3NzaDAxx2E"')
+        gitlab_keys.send :add_key
+      end
+
+      it "should return true" do
+        gitlab_keys.send(:add_key).should be_true
+      end
     end
   end
 
   describe :rm_key do
     let(:gitlab_keys) { build_gitlab_keys('rm-key', 'key-741', 'ssh-rsa AAAAB3NzaDAxx2E') }
-    let(:temp_file) { mock(:temp_file, path: 'tmp_path') }
-    before { Tempfile.should_receive(:open).and_yield(temp_file) }
 
-    it "should receive valid cmd" do
-      auth_file = GitlabConfig.new.auth_file
-      valid_cmd = "sed '/shell key-741\"/d' #{auth_file} > tmp_path && mv tmp_path #{auth_file}"
-      gitlab_keys.should_receive(:system).with(valid_cmd)
+    it "removes the right line" do
+      create_authorized_keys_fixture
+      other_line = "command=\"#{GitlabConfig.new.gitlab_shell_path}/bin/gitlab-shell key-742\",options ssh-rsa AAAAB3NzaDAxx2E"
+      open(tmp_authorized_keys_path, 'a') do |auth_file|
+        auth_file.puts "command=\"#{GitlabConfig.new.gitlab_shell_path}/bin/gitlab-shell key-741\",options ssh-rsa AAAAB3NzaDAxx2E"
+        auth_file.puts other_line
+      end
       gitlab_keys.send :rm_key
+      File.read(tmp_authorized_keys_path).should == "existing content\n#{other_line}\n"
     end
 
-    it "should log an rm-key event" do
-      $logger.should_receive(:info).with('Removing key key-741')
-      gitlab_keys.send :rm_key
+    context "without file writing" do
+      before { Tempfile.stub(:open) }
+
+      it "should log an rm-key event" do
+        $logger.should_receive(:info).with('Removing key key-741')
+        gitlab_keys.send :rm_key
+      end
+
+      it "should return true" do
+        gitlab_keys.send(:rm_key).should be_true
+      end
+    end
+  end
+
+  describe :clear do
+    let(:gitlab_keys) { build_gitlab_keys('clear') }
+
+    it "should return true" do
+      gitlab_keys.stub(:open)
+      gitlab_keys.send(:clear).should be_true
     end
   end
 
@@ -87,4 +113,14 @@ def argv(*args)
       ARGV[i] = arg
     end
   end
+
+  def create_authorized_keys_fixture
+    FileUtils.mkdir_p(File.dirname(tmp_authorized_keys_path))
+    open(tmp_authorized_keys_path, 'w') { |file| file.puts('existing content') }
+    gitlab_keys.stub(auth_file: tmp_authorized_keys_path)
+  end
+
+  def tmp_authorized_keys_path
+    File.join(GitlabConfig.new.gitlab_shell_path, 'tmp', 'authorized_keys')
+  end
 end