Commit 022242c3 authored by Douglas Barbosa Alexandre's avatar Douglas Barbosa Alexandre

Merge branch '25301-git-2.11-force-push-bug' into 'master'

Accept environment variables from the `pre-receive` script

## Summary

1. Starting version 2.11, git changed the way the pre-receive flow works.
  - Previously, the new potential objects would be added to the main repo. If the pre-receive passes, the new objects stay in the repo but are linked up. If the pre-receive fails, the new objects stay orphaned in the repo, and are cleaned up during the next `git gc`.
  - In 2.11, the new potential objects are added to a temporary "alternate object directory", that git creates for this purpose. If the pre-receive passes, the objects from the alternate object directory are migrated to the main repo. If the pre-receive fails the alternate object directory is simply deleted.
2. In our workflow, the pre-recieve script (in `gitlab-shell`) calls the
   `/allowed` endpoint, which calls out directly to git to perform
   various checks. These direct calls to git do _not_ have the necessary
   environment variables set which allow access to the "alternate object
   directory" (explained above). Therefore these calls to git are not able to
   access any of the new potential objects to be added during this push.

3. We fix this by accepting the relevant environment variables
   (`GIT_ALTERNATE_OBJECT_DIRECTORIES`, `GIT_OBJECT_DIRECTORY`, and
   `GIT_QUARANTINE_PATH`) on the `/allowed` endpoint, and then include
   these environment variables while calling out to git.

4. This commit includes these environment variables while making the "force
   push" check.

## Issue Numbers

- Closes #25301 (assuming the corresponding `gitlab-shell` MR has been merged in first)
- Corresponding `gitlab-shell` MR: gitlab-org/gitlab-shell!112
- Corresponding EE MR: gitlab-org/gitlab-ee!964

## Tasks

-  [#25301/!7967/!112] Git version 2.11.0 - Can't push to protected branch as master or developer
    - [x]  Investigate
    - [x]  Implementation
        - [x]  `force_push.rb` should use the relevant environment variables
        - [x]  Any other instances of `/allowed` calling out to git directly? 
        - [x]  Verify that the fix works over SSH as well
        - [x]  Can we trim the number of env variables? Do we need all 3?
        - [x]  Whitelist variables. Server shouldn't pass through _any_ env variable passed in
        - [x]  Any security implications?
        - [x]  Check for force push return code
        - [x]  Shouldn't be able to opt-out from the force push check by passing an env variable
    - [x]  Tests
        - [x]  CE
            - [x]  Added
            - [x]  Passing
        - [x]  Shell
            - [x]  Added
            - [x]  Passing
    - [x]  Meta
        - [x]  CHANGELOG entry created
        - [x]  Branch has no merge conflicts with `master`
        - [x]  Squashed related commits together
        - [x]  EE merge request
    - [x]  Review
        - [x]  Endboss
    - [ ]  Follow-up
        - [x]  Make sure EE is working as expected
        - [x]  [CE] Gitlab changes without gitlab-shell changes shouldn't raise any exceptions
        - [x]  [CE] Gitlab-shell changes without gitlab changes shouldn't raise any exceptions
        - [x]  [EE] Gitlab changes without gitlab-shell changes shouldn't raise any exceptions
        - [x]  [EE] Gitlab-shell changes without gitlab changes shouldn't raise any exceptions
    - [ ]  Wait for merge
        - [ ]  CE
        - [ ]  EE
        - [x]  Shell


See merge request !7967
parents 2c49c1af e394d287
---
title: Accept environment variables from the `pre-receive` script
merge_request: 7967
author:
...@@ -52,6 +52,14 @@ module API ...@@ -52,6 +52,14 @@ module API
:push_code :push_code
] ]
end end
def parse_allowed_environment_variables
return if params[:env].blank?
JSON.parse(params[:env])
rescue JSON::ParserError
end
end end
end end
end end
...@@ -32,7 +32,11 @@ module API ...@@ -32,7 +32,11 @@ module API
if wiki? if wiki?
Gitlab::GitAccessWiki.new(actor, project, protocol, authentication_abilities: ssh_authentication_abilities) Gitlab::GitAccessWiki.new(actor, project, protocol, authentication_abilities: ssh_authentication_abilities)
else else
Gitlab::GitAccess.new(actor, project, protocol, authentication_abilities: ssh_authentication_abilities) Gitlab::GitAccess.new(actor,
project,
protocol,
authentication_abilities: ssh_authentication_abilities,
env: parse_allowed_environment_variables)
end end
access_status = access.check(params[:action], params[:changes]) access_status = access.check(params[:action], params[:changes])
......
...@@ -3,11 +3,12 @@ module Gitlab ...@@ -3,11 +3,12 @@ module Gitlab
class ChangeAccess class ChangeAccess
attr_reader :user_access, :project attr_reader :user_access, :project
def initialize(change, user_access:, project:) def initialize(change, user_access:, project:, env: {})
@oldrev, @newrev, @ref = change.values_at(:oldrev, :newrev, :ref) @oldrev, @newrev, @ref = change.values_at(:oldrev, :newrev, :ref)
@branch_name = Gitlab::Git.branch_name(@ref) @branch_name = Gitlab::Git.branch_name(@ref)
@user_access = user_access @user_access = user_access
@project = project @project = project
@env = env
end end
def exec def exec
...@@ -68,7 +69,7 @@ module Gitlab ...@@ -68,7 +69,7 @@ module Gitlab
end end
def forced_push? def forced_push?
Gitlab::Checks::ForcePush.force_push?(@project, @oldrev, @newrev) Gitlab::Checks::ForcePush.force_push?(@project, @oldrev, @newrev, env: @env)
end end
def matching_merge_request? def matching_merge_request?
......
module Gitlab module Gitlab
module Checks module Checks
class ForcePush class ForcePush
def self.force_push?(project, oldrev, newrev) def self.force_push?(project, oldrev, newrev, env: {})
return false if project.empty_repo? return false if project.empty_repo?
# Created or deleted branch # Created or deleted branch
if Gitlab::Git.blank_ref?(oldrev) || Gitlab::Git.blank_ref?(newrev) if Gitlab::Git.blank_ref?(oldrev) || Gitlab::Git.blank_ref?(newrev)
false false
else else
missed_ref, _ = Gitlab::Popen.popen(%W(#{Gitlab.config.git.bin_path} --git-dir=#{project.repository.path_to_repo} rev-list --max-count=1 #{oldrev} ^#{newrev})) missed_ref, exit_status = Gitlab::Git::RevList.new(oldrev, newrev, project: project, env: env).execute
missed_ref.present?
if exit_status == 0
missed_ref.present?
else
raise "Got a non-zero exit code while calling out to `git rev-list` in the force-push check."
end
end end
end end
end end
......
module Gitlab
module Git
class RevList
attr_reader :project, :env
ALLOWED_VARIABLES = %w[GIT_OBJECT_DIRECTORY GIT_ALTERNATE_OBJECT_DIRECTORIES].freeze
def initialize(oldrev, newrev, project:, env: nil)
@project = project
@env = env.presence || {}
@args = [Gitlab.config.git.bin_path,
"--git-dir=#{project.repository.path_to_repo}",
"rev-list",
"--max-count=1",
oldrev,
"^#{newrev}"]
end
def execute
Gitlab::Popen.popen(@args, nil, parse_environment_variables)
end
def valid?
environment_variables.all? do |(name, value)|
value.start_with?(project.repository.path_to_repo)
end
end
private
def parse_environment_variables
return {} unless valid?
environment_variables
end
def environment_variables
@environment_variables ||= env.slice(*ALLOWED_VARIABLES)
end
end
end
end
...@@ -17,12 +17,13 @@ module Gitlab ...@@ -17,12 +17,13 @@ module Gitlab
attr_reader :actor, :project, :protocol, :user_access, :authentication_abilities attr_reader :actor, :project, :protocol, :user_access, :authentication_abilities
def initialize(actor, project, protocol, authentication_abilities:) def initialize(actor, project, protocol, authentication_abilities:, env: {})
@actor = actor @actor = actor
@project = project @project = project
@protocol = protocol @protocol = protocol
@authentication_abilities = authentication_abilities @authentication_abilities = authentication_abilities
@user_access = UserAccess.new(user, project: project) @user_access = UserAccess.new(user, project: project)
@env = env
end end
def check(cmd, changes) def check(cmd, changes)
...@@ -103,7 +104,7 @@ module Gitlab ...@@ -103,7 +104,7 @@ module Gitlab
end end
def change_access_check(change) def change_access_check(change)
Checks::ChangeAccess.new(change, user_access: user_access, project: project).exec Checks::ChangeAccess.new(change, user_access: user_access, project: project, env: @env).exec
end end
def protocol_allowed? def protocol_allowed?
......
...@@ -5,13 +5,13 @@ module Gitlab ...@@ -5,13 +5,13 @@ module Gitlab
module Popen module Popen
extend self extend self
def popen(cmd, path = nil) def popen(cmd, path = nil, vars = {})
unless cmd.is_a?(Array) unless cmd.is_a?(Array)
raise "System commands must be given as an array of strings" raise "System commands must be given as an array of strings"
end end
path ||= Dir.pwd path ||= Dir.pwd
vars = { "PWD" => path } vars['PWD'] = path
options = { chdir: path } options = { chdir: path }
unless File.directory?(path) unless File.directory?(path)
......
require 'spec_helper'
describe Gitlab::Checks::ChangeAccess, lib: true do
let(:project) { create(:project) }
context "exit code checking" do
it "does not raise a runtime error if the `popen` call to git returns a zero exit code" do
allow(Gitlab::Popen).to receive(:popen).and_return(['normal output', 0])
expect { Gitlab::Checks::ForcePush.force_push?(project, 'oldrev', 'newrev') }.not_to raise_error
end
it "raises a runtime error if the `popen` call to git returns a non-zero exit code" do
allow(Gitlab::Popen).to receive(:popen).and_return(['error', 1])
expect { Gitlab::Checks::ForcePush.force_push?(project, 'oldrev', 'newrev') }.to raise_error(RuntimeError)
end
end
end
require 'spec_helper'
describe Gitlab::Git::RevList, lib: true do
let(:project) { create(:project) }
context "validations" do
described_class::ALLOWED_VARIABLES.each do |var|
context var do
it "accepts values starting with the project repo path" do
env = { var => "#{project.repository.path_to_repo}/objects" }
rev_list = described_class.new('oldrev', 'newrev', project: project, env: env)
expect(rev_list).to be_valid
end
it "rejects values starting not with the project repo path" do
env = { var => "/some/other/path" }
rev_list = described_class.new('oldrev', 'newrev', project: project, env: env)
expect(rev_list).not_to be_valid
end
it "rejects values containing the project repo path but not starting with it" do
env = { var => "/some/other/path/#{project.repository.path_to_repo}" }
rev_list = described_class.new('oldrev', 'newrev', project: project, env: env)
expect(rev_list).not_to be_valid
end
end
end
end
context "#execute" do
let(:env) { { "GIT_OBJECT_DIRECTORY" => project.repository.path_to_repo } }
let(:rev_list) { Gitlab::Git::RevList.new('oldrev', 'newrev', project: project, env: env) }
it "calls out to `popen` without environment variables if the record is invalid" do
allow(rev_list).to receive(:valid?).and_return(false)
expect(Open3).to receive(:popen3).with(hash_excluding(env), any_args)
rev_list.execute
end
it "calls out to `popen` with environment variables if the record is valid" do
allow(rev_list).to receive(:valid?).and_return(true)
expect(Open3).to receive(:popen3).with(hash_including(env), any_args)
rev_list.execute
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