Verified Commit 864da5a4 authored by Marin Jankovski's avatar Marin Jankovski Committed by Rémy Coutable

Merge branch 'storage-directory-validation-fixes'

Signed-off-by: Rémy Coutable's avatarRémy Coutable <remy@rymai.me>
parent 7cfa764f
......@@ -7,6 +7,7 @@ omnibus-gitlab repository.
- Add support for registry debug addr configuration
- Add support for configuring workhorse's api limiting
- Fix unsetting the sticky bit for storage directory permissions and improved error messages
- Fixed a bug with disabling registry storage deletion
- Support specifying a post reconfigure script to run in the docker container
- Updated cacerts.pem to 2016-09-14 version
......
......@@ -49,7 +49,11 @@ class StorageDirectoryHelper
# Set the correct mode on the directory, run using the euid if target_owner
# has write access, otherwise use root
run_command("chmod #{@target_mode} #{path}", use_euid: writable?(path)) if @target_mode
if @target_mode
# Prepend a 0 to force an octal set when 4 bits have been passed in. eg: 2755 or 0700
mode = @target_mode.length == 4 ? "0#{@target_mode}" : @target_mode
run_command("chmod #{mode} #{path}", use_euid: writable?(path))
end
# Set the group on the directory, run using the euid if target_owner has
# write access, otherwise use root
......@@ -84,13 +88,9 @@ class StorageDirectoryHelper
end
def validate(path, throw_error: false)
# Test that directory is in expected state. The root user may not have
# execute permissions to the directory, but the target_owner will in the
# success case, so always use the euid to run the command
run_command(test_stat_cmd(path), use_euid: true, throw_error: throw_error).exitstatus == 0
end
commands = ["[ -d \"#{path}\" ]"]
commands_info = ["Failed expecting \"#{path}\" to be a directory."]
def test_stat_cmd(path)
format_string = '%U'
expect_string = "#{@target_owner}"
......@@ -99,11 +99,29 @@ class StorageDirectoryHelper
expect_string << ":#{@target_group}"
end
commands << "[ \"$(stat --printf='#{format_string}' $(readlink -f #{path}))\" = '#{expect_string}' ]"
commands_info << "Failed asserting that ownership of \"#{path}\" was #{expect_string}"
if @target_mode
format_string << ' %04a'
expect_string << " #{@target_mode}"
commands << "[ \"$(stat --printf='%04a' $(readlink -f #{path}) | grep -Po '.{#{@target_mode.length}}$')\" = '#{@target_mode}' ]"
commands_info << "Failed asserting that mode permissions on \"#{path}\" is #{@target_mode}"
end
"test -d \"#{path}\" -a \"$(stat --printf='#{format_string}' $(readlink -f #{path}))\" = '#{expect_string}'"
result = true
commands.each_index do |index|
result = result && validate_command(commands[index], throw_error: throw_error, error_message: commands_info[index])
break unless result
end
result
end
def validate_command(cmd, throw_error: false, error_message: nil)
# Test that directory is in expected state. The root user may not have
# execute permissions to the directory, but the target_owner will in the
# success case, so always use the euid to run the command, and use a custom error message
cmd = run_command("set -x && #{cmd}", use_euid: true, throw_error: false)
cmd.invalid!(error_message) if cmd.exitstatus != 0 && throw_error
cmd.exitstatus == 0
end
end
......@@ -36,7 +36,8 @@ RSpec.configure do |config|
stub_command(%r{\(test -f /var/opt/gitlab/gitlab-rails/upgrade-status/db-migrate-\h+-\) && \(cat /var/opt/gitlab/gitlab-rails/upgrade-status/db-migrate-\h+- | grep -Fx 0\)}).and_return('')
stub_command("getenforce | grep Disabled").and_return(true)
stub_command("semodule -l | grep '^#gitlab-7.2.0-ssh-keygen\\s'").and_return(true)
stub_command(%r{test \-d "[^"]\" \-a "\$\(stat \-\-printf='[^']*' \$\(readlink -f /[^\)]*\)\)" = }).and_return(false)
stub_command(%r{set \-x \&\& \[ \-d "[^"]\" \]}).and_return(false)
stub_command(%r{set \-x \&\& \[ "\$\(stat \-\-printf='[^']*' \$\(readlink -f /[^\)]*\)\) }).and_return(false)
allow_any_instance_of(Chef::Recipe).to receive(:system).with('/sbin/init --version | grep upstart')
allow_any_instance_of(Chef::Recipe).to receive(:system).with('systemctl | grep "\-\.mount"')
end
......
require 'chef_helper'
describe StorageDirectoryHelper do
let(:success_shell) do
shell = instance_double(Mixlib::ShellOut)
allow(shell).to receive(:exitstatus).and_return(0)
shell
end
let(:fail_shell) do
shell = instance_double(Mixlib::ShellOut)
allow(shell).to receive(:exitstatus).and_return(1)
shell
end
before { allow(Gitlab).to receive(:[]).and_call_original }
describe :validate do
context 'owner provided' do
subject { ::StorageDirectoryHelper.new('git', nil, nil) }
it 'checks directory and owner and succeeds' do
expect(subject).to receive(:run_command).with("set -x && [ -d \"/tmp/validate\" ]", any_args).and_return(success_shell)
expect(subject).to receive(:run_command)
.with("set -x && [ \"$(stat --printf='%U' $(readlink -f /tmp/validate))\" = 'git' ]", any_args).and_return(success_shell)
expect(subject.validate('/tmp/validate')).to eq(true)
end
it 'fails when path is not a directory' do
expect(subject).to receive(:run_command).with("set -x && [ -d \"/tmp/validate\" ]", any_args).and_return(fail_shell)
expect(subject).to_not receive(:run_command)
.with("set -x && [ \"$(stat --printf='%U' $(readlink -f /tmp/validate))\" = 'git' ]", any_args)
expect(subject.validate('/tmp/validate')).to eq(false)
end
it 'fails when owner does not match' do
expect(subject).to receive(:run_command).with("set -x && [ -d \"/tmp/validate\" ]", any_args).and_return(success_shell)
expect(subject).to receive(:run_command)
.with("set -x && [ \"$(stat --printf='%U' $(readlink -f /tmp/validate))\" = 'git' ]", any_args).and_return(fail_shell)
expect(subject.validate('/tmp/validate')).to eq(false)
end
end
context 'owner and group provided' do
subject { ::StorageDirectoryHelper.new('git', 'root', nil) }
it 'checks directory, owner and group and succeeds' do
expect(subject).to receive(:run_command).with("set -x && [ -d \"/tmp/validate\" ]", any_args).and_return(success_shell)
expect(subject).to receive(:run_command)
.with("set -x && [ \"$(stat --printf='%U:%G' $(readlink -f /tmp/validate))\" = 'git:root' ]", any_args).and_return(success_shell)
expect(subject.validate('/tmp/validate')).to eq(true)
end
it 'fails when path is not a directory' do
expect(subject).to receive(:run_command).with("set -x && [ -d \"/tmp/validate\" ]", any_args).and_return(fail_shell)
expect(subject).to_not receive(:run_command)
.with("set -x && [ \"$(stat --printf='%U:%G' $(readlink -f /tmp/validate))\" = 'git:root' ]", any_args)
expect(subject.validate('/tmp/validate')).to eq(false)
end
it 'fails when group does not match' do
expect(subject).to receive(:run_command).with("set -x && [ -d \"/tmp/validate\" ]", any_args).and_return(success_shell)
expect(subject).to receive(:run_command)
.with("set -x && [ \"$(stat --printf='%U:%G' $(readlink -f /tmp/validate))\" = 'git:root' ]", any_args).and_return(fail_shell)
expect(subject.validate('/tmp/validate')).to eq(false)
end
end
context 'owner and permission mode provided' do
subject { ::StorageDirectoryHelper.new('git', nil, '700') }
it 'checks directory, owner and permissions and succeeds' do
expect(subject).to receive(:run_command).with("set -x && [ -d \"/tmp/validate\" ]", any_args).and_return(success_shell)
expect(subject).to receive(:run_command)
.with("set -x && [ \"$(stat --printf='%U' $(readlink -f /tmp/validate))\" = 'git' ]", any_args).and_return(success_shell)
expect(subject).to receive(:run_command)
.with("set -x && [ \"$(stat --printf='%04a' $(readlink -f /tmp/validate) | grep -Po '.{3}$')\" = '700' ]", any_args).and_return(success_shell)
expect(subject.validate('/tmp/validate')).to eq(true)
end
it 'fails when path is not a directory' do
expect(subject).to receive(:run_command).with("set -x && [ -d \"/tmp/validate\" ]", any_args).and_return(fail_shell)
expect(subject).to_not receive(:run_command)
.with("set -x && [ \"$(stat --printf='%U:%G' $(readlink -f /tmp/validate))\" = 'git:root' ]", any_args)
expect(subject).to_not receive(:run_command)
.with("set -x && [ \"$(stat --printf='%04a' $(readlink -f /tmp/validate) | grep -Po '.{3}$')\" = '700' ]", any_args)
expect(subject.validate('/tmp/validate')).to eq(false)
end
it 'fails when owner does not match' do
expect(subject).to receive(:run_command).with("set -x && [ -d \"/tmp/validate\" ]", any_args).and_return(success_shell)
expect(subject).to receive(:run_command)
.with("set -x && [ \"$(stat --printf='%U' $(readlink -f /tmp/validate))\" = 'git' ]", any_args).and_return(fail_shell)
expect(subject).to_not receive(:run_command)
.with("set -x && [ \"$(stat --printf='%04a' $(readlink -f /tmp/validate) | grep -Po '.{3}$')\" = '700' ]", any_args)
expect(subject.validate('/tmp/validate')).to eq(false)
end
it 'fails when permissions do not match' do
expect(subject).to receive(:run_command).with("set -x && [ -d \"/tmp/validate\" ]", any_args).and_return(success_shell)
expect(subject).to receive(:run_command)
.with("set -x && [ \"$(stat --printf='%U' $(readlink -f /tmp/validate))\" = 'git' ]", any_args).and_return(success_shell)
expect(subject).to receive(:run_command)
.with("set -x && [ \"$(stat --printf='%04a' $(readlink -f /tmp/validate) | grep -Po '.{3}$')\" = '700' ]", any_args).and_return(fail_shell)
expect(subject.validate('/tmp/validate')).to eq(false)
end
end
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