Commit bad08fbe authored by Michael Kozono's avatar Michael Kozono

Move CI access logic into GitAccess

parent b3874294
...@@ -106,4 +106,8 @@ module LfsRequest ...@@ -106,4 +106,8 @@ module LfsRequest
def objects def objects
@objects ||= (params[:objects] || []).to_a @objects ||= (params[:objects] || []).to_a
end end
def has_authentication_ability?(capability)
(authentication_abilities || []).include?(capability)
end
end end
...@@ -128,28 +128,10 @@ class Projects::GitHttpClientController < Projects::ApplicationController ...@@ -128,28 +128,10 @@ class Projects::GitHttpClientController < Projects::ApplicationController
@authentication_result = Gitlab::Auth.find_for_git_client( @authentication_result = Gitlab::Auth.find_for_git_client(
login, password, project: project, ip: request.ip) login, password, project: project, ip: request.ip)
return false unless @authentication_result.success? @authentication_result.success?
if download_request?
authentication_has_download_access?
else
authentication_has_upload_access?
end
end end
def ci? def ci?
authentication_result.ci?(project) authentication_result.ci?(project)
end end
def authentication_has_download_access?
has_authentication_ability?(:download_code) || has_authentication_ability?(:build_download_code)
end
def authentication_has_upload_access?
has_authentication_ability?(:push_code)
end
def has_authentication_ability?(capability)
(authentication_abilities || []).include?(capability)
end
end end
...@@ -67,20 +67,24 @@ class Projects::GitHttpController < Projects::GitHttpClientController ...@@ -67,20 +67,24 @@ class Projects::GitHttpController < Projects::GitHttpClientController
end end
def render_denied def render_denied
if user && can?(user, :read_project, project) if access_check.message == Gitlab::GitAccess::ERROR_MESSAGES[:project_not_found]
render plain: access_check.message, status: :forbidden render plain: access_check.message, status: :not_found
else else
# Do not leak information about project existence render plain: access_check.message, status: :forbidden
render_not_found
end end
end end
def upload_pack_allowed? def upload_pack_allowed?
access_check.allowed? || ci? access_check.allowed?
end end
def access def access
@access ||= access_klass.new(user, project, 'http', authentication_abilities: authentication_abilities) @access ||= access_klass.new(access_actor, project, 'http', authentication_abilities: authentication_abilities)
end
def access_actor
return user if user
return :ci if ci?
end end
def access_check def access_check
......
...@@ -63,6 +63,11 @@ module Gitlab ...@@ -63,6 +63,11 @@ module Gitlab
authentication_abilities.include?(:build_download_code) && user_access.can_do_action?(:build_download_code) authentication_abilities.include?(:build_download_code) && user_access.can_do_action?(:build_download_code)
end end
# Allow generic CI (build without a user) for backwards compatibility
def ci_can_download_code?
authentication_abilities.include?(:build_download_code) && ci?
end
def protocol_allowed? def protocol_allowed?
Gitlab::ProtocolAccess.allowed?(protocol) Gitlab::ProtocolAccess.allowed?(protocol)
end end
...@@ -115,6 +120,7 @@ module Gitlab ...@@ -115,6 +120,7 @@ module Gitlab
return if deploy_key? return if deploy_key?
passed = user_can_download_code? || passed = user_can_download_code? ||
ci_can_download_code? ||
build_can_download_code? || build_can_download_code? ||
guest_can_download_code? guest_can_download_code?
...@@ -184,11 +190,17 @@ module Gitlab ...@@ -184,11 +190,17 @@ module Gitlab
actor.is_a?(DeployKey) actor.is_a?(DeployKey)
end end
def ci?
actor == :ci
end
def can_read_project? def can_read_project?
if deploy_key if deploy_key?
deploy_key.has_access_to?(project) deploy_key.has_access_to?(project)
elsif user elsif user
user.can?(:read_project, project) user.can?(:read_project, project)
elsif ci?
true # allow CI (build without a user) for backwards compatibility
end || Guest.can?(:read_project, project) end || Guest.can?(:read_project, project)
end end
...@@ -213,10 +225,12 @@ module Gitlab ...@@ -213,10 +225,12 @@ module Gitlab
case actor case actor
when User when User
actor actor
when DeployKey
nil
when Key when Key
actor.user actor.user
when DeployKey
nil
when :ci
nil
end end
end end
......
require 'spec_helper' require 'spec_helper'
describe Gitlab::GitAccess, lib: true do describe Gitlab::GitAccess, lib: true do
let(:pull_access_check) { access.check('git-upload-pack', '_any') }
let(:push_access_check) { access.check('git-receive-pack', '_any') }
let(:access) { Gitlab::GitAccess.new(actor, project, protocol, authentication_abilities: authentication_abilities) } let(:access) { Gitlab::GitAccess.new(actor, project, protocol, authentication_abilities: authentication_abilities) }
let(:project) { create(:project, :repository) } let(:project) { create(:project, :repository) }
let(:user) { create(:user) } let(:user) { create(:user) }
...@@ -51,7 +53,123 @@ describe Gitlab::GitAccess, lib: true do ...@@ -51,7 +53,123 @@ describe Gitlab::GitAccess, lib: true do
end end
end end
describe '#check with commands disabled' do describe '#check_project_accessibility!' do
context 'when the project exists' do
context 'when actor exists' do
context 'when actor is a DeployKey' do
let(:deploy_key) { create(:deploy_key, user: user, can_push: true) }
let(:actor) { deploy_key }
context 'when the DeployKey has access to the project' do
before { deploy_key.projects << project }
it 'allows pull access' do
expect(pull_access_check.allowed?).to be_truthy
end
it 'allows push access' do
expect(push_access_check.allowed?).to be_truthy
end
end
context 'when the Deploykey does not have access to the project' do
it 'blocks pulls with "not found"' do
expect(pull_access_check.allowed?).to be_falsey
expect(pull_access_check.message).to eq('The project you were looking for could not be found.')
end
it 'blocks pushes with "not found"' do
expect(push_access_check.allowed?).to be_falsey
expect(push_access_check.message).to eq('The project you were looking for could not be found.')
end
end
end
context 'when actor is a User' do
context 'when the User can read the project' do
before { project.team << [user, :master] }
it 'allows pull access' do
expect(pull_access_check.allowed?).to be_truthy
end
it 'allows push access' do
expect(push_access_check.allowed?).to be_truthy
end
end
context 'when the User cannot read the project' do
it 'blocks pulls with "not found"' do
expect(pull_access_check.allowed?).to be_falsey
expect(pull_access_check.message).to eq('The project you were looking for could not be found.')
end
it 'blocks pushes with "not found"' do
expect(push_access_check.allowed?).to be_falsey
expect(push_access_check.message).to eq('The project you were looking for could not be found.')
end
end
end
# For backwards compatibility
context 'when actor is :ci' do
let(:actor) { :ci }
let(:authentication_abilities) { build_authentication_abilities }
it 'allows pull access' do
expect(pull_access_check.allowed?).to be_truthy
end
it 'does not block pushes with "not found"' do
expect(push_access_check.allowed?).to be_falsey
expect(push_access_check.message).to eq('You are not allowed to upload code for this project.')
end
end
end
context 'when actor is nil' do
let(:actor) { nil }
context 'when guests can read the project' do
let(:project) { create(:project, :repository, :public) }
it 'allows pull access' do
expect(pull_access_check.allowed?).to be_truthy
end
it 'does not block pushes with "not found"' do
expect(push_access_check.allowed?).to be_falsey
expect(push_access_check.message).to eq('You are not allowed to upload code for this project.')
end
end
context 'when guests cannot read the project' do
it 'blocks pulls with "not found"' do
expect(pull_access_check.allowed?).to be_falsey
expect(pull_access_check.message).to eq('The project you were looking for could not be found.')
end
it 'blocks pushes with "not found"' do
expect(push_access_check.allowed?).to be_falsey
expect(push_access_check.message).to eq('The project you were looking for could not be found.')
end
end
end
end
context 'when the project is nil' do
let(:project) { nil }
it 'blocks any command with "not found"' do
expect(pull_access_check.allowed?).to be_falsey
expect(pull_access_check.message).to eq('The project you were looking for could not be found.')
expect(push_access_check.allowed?).to be_falsey
expect(push_access_check.message).to eq('The project you were looking for could not be found.')
end
end
end
describe '#check_command_disabled!' do
before { project.team << [user, :master] } before { project.team << [user, :master] }
context 'over http' do context 'over http' do
...@@ -219,6 +337,14 @@ describe Gitlab::GitAccess, lib: true do ...@@ -219,6 +337,14 @@ describe Gitlab::GitAccess, lib: true do
end end
end end
end end
describe 'generic CI (build without a user)' do
let(:actor) { :ci }
context 'pull code' do
it { expect(subject).to be_allowed }
end
end
end end
end end
......
...@@ -489,29 +489,41 @@ describe 'Git HTTP requests', lib: true do ...@@ -489,29 +489,41 @@ describe 'Git HTTP requests', lib: true do
end end
context "when a gitlab ci token is provided" do context "when a gitlab ci token is provided" do
let(:project) { create(:project, :repository) }
let(:build) { create(:ci_build, :running) } let(:build) { create(:ci_build, :running) }
let(:project) { build.project }
let(:other_project) { create(:empty_project) } let(:other_project) { create(:empty_project) }
before do
build.update!(project: project) # can't associate it on factory create
end
context 'when build created by system is authenticated' do context 'when build created by system is authenticated' do
let(:path) { "#{project.path_with_namespace}.git" } let(:path) { "#{project.path_with_namespace}.git" }
let(:env) { { user: 'gitlab-ci-token', password: build.token } } let(:env) { { user: 'gitlab-ci-token', password: build.token } }
it_behaves_like 'pulls are allowed' it_behaves_like 'pulls are allowed'
# TODO Verify this is desired behavior # A non-401 here is not an information leak since the system is
it "rejects pushes with 401 Unauthorized (no project existence information leak)" do # "authenticated" as CI using the correct token. It does not have
# push access, so pushes should be rejected as forbidden, and giving
# a reason is fine.
#
# We know for sure it is not an information leak since pulls using
# the build token must be allowed.
it "rejects pushes with 403 Forbidden" do
push_get(path, env) push_get(path, env)
expect(response).to have_http_status(:unauthorized) expect(response).to have_http_status(:forbidden)
expect(response.body).to eq(git_access_error(:upload))
end end
# TODO Verify this is desired behavior. Should be 403 Forbidden? # We are "authenticated" as CI using a valid token here. But we are
# not authorized to see any other project, so return "not found".
it "rejects pulls for other project with 404 Not Found" do it "rejects pulls for other project with 404 Not Found" do
clone_get("#{other_project.path_with_namespace}.git", env) clone_get("#{other_project.path_with_namespace}.git", env)
expect(response).to have_http_status(:not_found) expect(response).to have_http_status(:not_found)
expect(response.body).to eq('TODO: What should this be?') expect(response.body).to eq(git_access_error(:project_not_found))
end end
end end
...@@ -522,31 +534,27 @@ describe 'Git HTTP requests', lib: true do ...@@ -522,31 +534,27 @@ describe 'Git HTTP requests', lib: true do
end end
shared_examples 'can download code only' do shared_examples 'can download code only' do
it 'downloads get status 200' do let(:path) { "#{project.path_with_namespace}.git" }
allow_any_instance_of(Repository). let(:env) { { user: 'gitlab-ci-token', password: build.token } }
to receive(:exists?).and_return(true)
clone_get "#{project.path_with_namespace}.git",
user: 'gitlab-ci-token', password: build.token
expect(response).to have_http_status(:ok) it_behaves_like 'pulls are allowed'
expect(response.content_type.to_s).to eq(Gitlab::Workhorse::INTERNAL_API_CONTENT_TYPE)
end
it 'downloads from non-existing repository and gets 403' do
allow_any_instance_of(Repository).
to receive(:exists?).and_return(false)
clone_get "#{project.path_with_namespace}.git", context 'when the repo does not exist' do
user: 'gitlab-ci-token', password: build.token let(:project) { create(:empty_project) }
it 'rejects pulls with 403 Forbidden' do
clone_get path, env
expect(response).to have_http_status(:forbidden) expect(response).to have_http_status(:forbidden)
expect(response.body).to eq(git_access_error(:no_repo))
end
end end
it 'uploads get status 403' do it 'rejects pushes with 403 Forbidden' do
push_get "#{project.path_with_namespace}.git", user: 'gitlab-ci-token', password: build.token push_get path, env
expect(response).to have_http_status(:unauthorized) expect(response).to have_http_status(:forbidden)
expect(response.body).to eq(git_access_error(:upload))
end end
end end
......
...@@ -52,14 +52,17 @@ module GitHttpHelpers ...@@ -52,14 +52,17 @@ module GitHttpHelpers
end end
def git_access_error(error_key) def git_access_error(error_key)
Gitlab::GitAccess::ERROR_MESSAGES[error_key] message = Gitlab::GitAccess::ERROR_MESSAGES[error_key]
message || raise("GitAccess error message key '#{error_key}' not found")
end end
def git_access_wiki_error(error_key) def git_access_wiki_error(error_key)
Gitlab::GitAccessWiki::ERROR_MESSAGES[error_key] message = Gitlab::GitAccessWiki::ERROR_MESSAGES[error_key]
message || raise("GitAccessWiki error message key '#{error_key}' not found")
end end
def change_access_error(error_key) def change_access_error(error_key)
Gitlab::Checks::ChangeAccess::ERROR_MESSAGES[error_key] message = Gitlab::Checks::ChangeAccess::ERROR_MESSAGES[error_key]
message || raise("ChangeAccess error message key '#{error_key}' not found")
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