diff --git a/app/controllers/concerns/lfs_request.rb b/app/controllers/concerns/lfs_request.rb index ae91e02488a90c90f0ad150f4cade066a8d0cf6d..2b6afaa6233a00f5105f227fdd4dbd07da916635 100644 --- a/app/controllers/concerns/lfs_request.rb +++ b/app/controllers/concerns/lfs_request.rb @@ -106,4 +106,8 @@ module LfsRequest def objects @objects ||= (params[:objects] || []).to_a end + + def has_authentication_ability?(capability) + (authentication_abilities || []).include?(capability) + end end diff --git a/app/controllers/projects/git_http_client_controller.rb b/app/controllers/projects/git_http_client_controller.rb index 44b0853e3e9d3442c66d1fb13981597bc87de0a6..7f3205a8001982acd3bb22aceb805b92730be7fd 100644 --- a/app/controllers/projects/git_http_client_controller.rb +++ b/app/controllers/projects/git_http_client_controller.rb @@ -128,28 +128,10 @@ class Projects::GitHttpClientController < Projects::ApplicationController @authentication_result = Gitlab::Auth.find_for_git_client( login, password, project: project, ip: request.ip) - return false unless @authentication_result.success? - - if download_request? - authentication_has_download_access? - else - authentication_has_upload_access? - end + @authentication_result.success? end def ci? authentication_result.ci?(project) 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 diff --git a/app/controllers/projects/git_http_controller.rb b/app/controllers/projects/git_http_controller.rb index e7b498599f25c96cf84530a990bcff122ce4ae11..2c2766cf623386d3da5c322d5b4c5dcb42c555d7 100644 --- a/app/controllers/projects/git_http_controller.rb +++ b/app/controllers/projects/git_http_controller.rb @@ -67,20 +67,24 @@ class Projects::GitHttpController < Projects::GitHttpClientController end def render_denied - if user && can?(user, :read_project, project) - render plain: access_check.message, status: :forbidden + if access_check.message == Gitlab::GitAccess::ERROR_MESSAGES[:project_not_found] + render plain: access_check.message, status: :not_found else - # Do not leak information about project existence - render_not_found + render plain: access_check.message, status: :forbidden end end def upload_pack_allowed? - access_check.allowed? || ci? + access_check.allowed? end 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 def access_check diff --git a/lib/gitlab/git_access.rb b/lib/gitlab/git_access.rb index 1d052ac9b339b29050f6853f74fbe1279c0a792b..1ffac5385c2cd21a9335198bb41a4eb72411032a 100644 --- a/lib/gitlab/git_access.rb +++ b/lib/gitlab/git_access.rb @@ -63,6 +63,11 @@ module Gitlab authentication_abilities.include?(:build_download_code) && user_access.can_do_action?(:build_download_code) 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? Gitlab::ProtocolAccess.allowed?(protocol) end @@ -115,6 +120,7 @@ module Gitlab return if deploy_key? passed = user_can_download_code? || + ci_can_download_code? || build_can_download_code? || guest_can_download_code? @@ -184,11 +190,17 @@ module Gitlab actor.is_a?(DeployKey) end + def ci? + actor == :ci + end + def can_read_project? - if deploy_key + if deploy_key? deploy_key.has_access_to?(project) elsif user user.can?(:read_project, project) + elsif ci? + true # allow CI (build without a user) for backwards compatibility end || Guest.can?(:read_project, project) end @@ -213,10 +225,12 @@ module Gitlab case actor when User actor - when DeployKey - nil when Key actor.user + when DeployKey + nil + when :ci + nil end end diff --git a/spec/lib/gitlab/git_access_spec.rb b/spec/lib/gitlab/git_access_spec.rb index 1033fef968463c228d3c1c5496a4ef100de544c9..0efe15856fc63256fcf41429b7b0fb6582d72f6a 100644 --- a/spec/lib/gitlab/git_access_spec.rb +++ b/spec/lib/gitlab/git_access_spec.rb @@ -1,6 +1,8 @@ require 'spec_helper' 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(:project) { create(:project, :repository) } let(:user) { create(:user) } @@ -51,7 +53,123 @@ describe Gitlab::GitAccess, lib: true do 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] } context 'over http' do @@ -219,6 +337,14 @@ describe Gitlab::GitAccess, lib: true do 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 diff --git a/spec/requests/git_http_spec.rb b/spec/requests/git_http_spec.rb index 080e2f12cd707fa26353ddac68d8950e538c04bb..ab7c56fcdf0a48fd5830fbd845575a4738b4dea8 100644 --- a/spec/requests/git_http_spec.rb +++ b/spec/requests/git_http_spec.rb @@ -489,29 +489,41 @@ describe 'Git HTTP requests', lib: true do end context "when a gitlab ci token is provided" do + let(:project) { create(:project, :repository) } let(:build) { create(:ci_build, :running) } - let(:project) { build.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 let(:path) { "#{project.path_with_namespace}.git" } let(:env) { { user: 'gitlab-ci-token', password: build.token } } it_behaves_like 'pulls are allowed' - # TODO Verify this is desired behavior - it "rejects pushes with 401 Unauthorized (no project existence information leak)" do + # A non-401 here is not an information leak since the system is + # "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) - expect(response).to have_http_status(:unauthorized) + expect(response).to have_http_status(:forbidden) + expect(response.body).to eq(git_access_error(:upload)) 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 clone_get("#{other_project.path_with_namespace}.git", env) 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 @@ -522,31 +534,27 @@ describe 'Git HTTP requests', lib: true do end shared_examples 'can download code only' do - it 'downloads get status 200' do - allow_any_instance_of(Repository). - to receive(:exists?).and_return(true) - - clone_get "#{project.path_with_namespace}.git", - user: 'gitlab-ci-token', password: build.token + let(:path) { "#{project.path_with_namespace}.git" } + let(:env) { { user: 'gitlab-ci-token', password: build.token } } - expect(response).to have_http_status(:ok) - 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) + it_behaves_like 'pulls are allowed' - clone_get "#{project.path_with_namespace}.git", - user: 'gitlab-ci-token', password: build.token + context 'when the repo does not exist' do + 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 - it 'uploads get status 403' do - push_get "#{project.path_with_namespace}.git", user: 'gitlab-ci-token', password: build.token + it 'rejects pushes with 403 Forbidden' do + 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 diff --git a/spec/support/git_http_helpers.rb b/spec/support/git_http_helpers.rb index d3d51560a9dc3e7f43bead635ebd1d6bbe31cabe..b8289e6c5f1a6099897c77cfdd9a872a21861547 100644 --- a/spec/support/git_http_helpers.rb +++ b/spec/support/git_http_helpers.rb @@ -52,14 +52,17 @@ module GitHttpHelpers end 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 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 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