Commit 55289c10 authored by Douwe Maan's avatar Douwe Maan Committed by Rémy Coutable

Merge branch 'fix-unathorized-cloning' into 'security'

Ensure external users are not able to clone disabled repositories.

Closes https://gitlab.com/gitlab-org/gitlab-ce/issues/23788

See merge request !2017
parent 4b5489e5
...@@ -21,10 +21,6 @@ class Projects::GitHttpClientController < Projects::ApplicationController ...@@ -21,10 +21,6 @@ class Projects::GitHttpClientController < Projects::ApplicationController
def authenticate_user def authenticate_user
@authentication_result = Gitlab::Auth::Result.new @authentication_result = Gitlab::Auth::Result.new
if project && project.public? && download_request?
return # Allow access
end
if allow_basic_auth? && basic_auth_provided? if allow_basic_auth? && basic_auth_provided?
login, password = user_name_and_password(request) login, password = user_name_and_password(request)
...@@ -41,6 +37,10 @@ class Projects::GitHttpClientController < Projects::ApplicationController ...@@ -41,6 +37,10 @@ class Projects::GitHttpClientController < Projects::ApplicationController
send_final_spnego_response send_final_spnego_response
return # Allow access return # Allow access
end end
elsif project && download_request? && Guest.can?(:download_code, project)
@authentication_result = Gitlab::Auth::Result.new(nil, project, :none, [:download_code])
return # Allow access
end end
send_challenges send_challenges
......
...@@ -78,11 +78,7 @@ class Projects::GitHttpController < Projects::GitHttpClientController ...@@ -78,11 +78,7 @@ class Projects::GitHttpController < Projects::GitHttpClientController
def upload_pack_allowed? def upload_pack_allowed?
return false unless Gitlab.config.gitlab_shell.upload_pack return false unless Gitlab.config.gitlab_shell.upload_pack
if user access_check.allowed? || ci?
access_check.allowed?
else
ci? || project.public?
end
end end
def access def access
......
...@@ -27,7 +27,7 @@ module LfsHelper ...@@ -27,7 +27,7 @@ module LfsHelper
def lfs_download_access? def lfs_download_access?
return false unless project.lfs_enabled? return false unless project.lfs_enabled?
project.public? || ci? || lfs_deploy_token? || user_can_download_code? || build_can_download_code? ci? || lfs_deploy_token? || user_can_download_code? || build_can_download_code?
end end
def user_can_download_code? def user_can_download_code?
......
class Guest
class << self
def can?(action, subject)
Ability.allowed?(nil, action, subject)
end
end
end
...@@ -76,7 +76,7 @@ module Auth ...@@ -76,7 +76,7 @@ module Auth
case requested_action case requested_action
when 'pull' when 'pull'
requested_project.public? || build_can_pull?(requested_project) || user_can_pull?(requested_project) build_can_pull?(requested_project) || user_can_pull?(requested_project)
when 'push' when 'push'
build_can_push?(requested_project) || user_can_push?(requested_project) build_can_push?(requested_project) || user_can_push?(requested_project)
else else
......
---
title: Ensure external users are not able to clone disabled repositories.
merge_request:
author:
...@@ -2,8 +2,18 @@ ...@@ -2,8 +2,18 @@
# class return an instance of `GitlabAccessStatus` # class return an instance of `GitlabAccessStatus`
module Gitlab module Gitlab
class GitAccess class GitAccess
UnauthorizedError = Class.new(StandardError)
ERROR_MESSAGES = {
upload: 'You are not allowed to upload code for this project.',
download: 'You are not allowed to download code from this project.',
deploy_key: 'Deploy keys are not allowed to push code.',
no_repo: 'A repository for this project does not exist yet.'
}
DOWNLOAD_COMMANDS = %w{ git-upload-pack git-upload-archive } DOWNLOAD_COMMANDS = %w{ git-upload-pack git-upload-archive }
PUSH_COMMANDS = %w{ git-receive-pack } PUSH_COMMANDS = %w{ git-receive-pack }
ALL_COMMANDS = DOWNLOAD_COMMANDS + PUSH_COMMANDS
attr_reader :actor, :project, :protocol, :user_access, :authentication_abilities attr_reader :actor, :project, :protocol, :user_access, :authentication_abilities
...@@ -16,56 +26,43 @@ module Gitlab ...@@ -16,56 +26,43 @@ module Gitlab
end end
def check(cmd, changes) def check(cmd, changes)
return build_status_object(false, "Git access over #{protocol.upcase} is not allowed") unless protocol_allowed? check_protocol!
check_active_user!
unless actor check_project_accessibility!
return build_status_object(false, "No user or key was provided.") check_command_existence!(cmd)
end
if user && !user_access.allowed?
return build_status_object(false, "Your account has been blocked.")
end
unless project && (user_access.can_read_project? || deploy_key_can_read_project?)
return build_status_object(false, 'The project you were looking for could not be found.')
end
case cmd case cmd
when *DOWNLOAD_COMMANDS when *DOWNLOAD_COMMANDS
download_access_check download_access_check
when *PUSH_COMMANDS when *PUSH_COMMANDS
push_access_check(changes) push_access_check(changes)
else
build_status_object(false, "The command you're trying to execute is not allowed.")
end end
build_status_object(true)
rescue UnauthorizedError => ex
build_status_object(false, ex.message)
end end
def download_access_check def download_access_check
if user if user
user_download_access_check user_download_access_check
elsif deploy_key elsif deploy_key.nil? && !Guest.can?(:download_code, project)
build_status_object(true) raise UnauthorizedError, ERROR_MESSAGES[:download]
else
raise 'Wrong actor'
end end
end end
def push_access_check(changes) def push_access_check(changes)
if user if user
user_push_access_check(changes) user_push_access_check(changes)
elsif deploy_key
build_status_object(false, "Deploy keys are not allowed to push code.")
else else
raise 'Wrong actor' raise UnauthorizedError, ERROR_MESSAGES[deploy_key ? :deploy_key : :upload]
end end
end end
def user_download_access_check def user_download_access_check
unless user_can_download_code? || build_can_download_code? unless user_can_download_code? || build_can_download_code?
return build_status_object(false, "You are not allowed to download code from this project.") raise UnauthorizedError, ERROR_MESSAGES[:download]
end end
build_status_object(true)
end end
def user_can_download_code? def user_can_download_code?
...@@ -78,15 +75,15 @@ module Gitlab ...@@ -78,15 +75,15 @@ module Gitlab
def user_push_access_check(changes) def user_push_access_check(changes)
unless authentication_abilities.include?(:push_code) unless authentication_abilities.include?(:push_code)
return build_status_object(false, "You are not allowed to upload code for this project.") raise UnauthorizedError, ERROR_MESSAGES[:upload]
end end
if changes.blank? if changes.blank?
return build_status_object(true) return # Allow access.
end end
unless project.repository.exists? unless project.repository.exists?
return build_status_object(false, "A repository for this project does not exist yet.") raise UnauthorizedError, ERROR_MESSAGES[:no_repo]
end end
changes_list = Gitlab::ChangesList.new(changes) changes_list = Gitlab::ChangesList.new(changes)
...@@ -96,11 +93,9 @@ module Gitlab ...@@ -96,11 +93,9 @@ module Gitlab
status = change_access_check(change) status = change_access_check(change)
unless status.allowed? unless status.allowed?
# If user does not have access to make at least one change - cancel all push # If user does not have access to make at least one change - cancel all push
return status raise UnauthorizedError, status.message
end end
end end
build_status_object(true)
end end
def change_access_check(change) def change_access_check(change)
...@@ -113,6 +108,30 @@ module Gitlab ...@@ -113,6 +108,30 @@ module Gitlab
private private
def check_protocol!
unless protocol_allowed?
raise UnauthorizedError, "Git access over #{protocol.upcase} is not allowed"
end
end
def check_active_user!
if user && !user_access.allowed?
raise UnauthorizedError, "Your account has been blocked."
end
end
def check_project_accessibility!
if project.blank? || !can_read_project?
raise UnauthorizedError, 'The project you were looking for could not be found.'
end
end
def check_command_existence!(cmd)
unless ALL_COMMANDS.include?(cmd)
raise UnauthorizedError, "The command you're trying to execute is not allowed."
end
end
def matching_merge_request?(newrev, branch_name) def matching_merge_request?(newrev, branch_name)
Checks::MatchingMergeRequest.new(newrev, branch_name, project).match? Checks::MatchingMergeRequest.new(newrev, branch_name, project).match?
end end
...@@ -130,6 +149,16 @@ module Gitlab ...@@ -130,6 +149,16 @@ module Gitlab
end end
end end
def can_read_project?
if user
user_access.can_read_project?
elsif deploy_key
deploy_key_can_read_project?
else
Guest.can?(:read_project, project)
end
end
protected protected
def user def user
......
...@@ -49,13 +49,17 @@ FactoryGirl.define do ...@@ -49,13 +49,17 @@ FactoryGirl.define do
end end
after(:create) do |project, evaluator| after(:create) do |project, evaluator|
# Builds and MRs can't have higher visibility level than repository access level.
builds_access_level = [evaluator.builds_access_level, evaluator.repository_access_level].min
merge_requests_access_level = [evaluator.merge_requests_access_level, evaluator.repository_access_level].min
project.project_feature. project.project_feature.
update_attributes( update_attributes!(
wiki_access_level: evaluator.wiki_access_level, wiki_access_level: evaluator.wiki_access_level,
builds_access_level: evaluator.builds_access_level, builds_access_level: builds_access_level,
snippets_access_level: evaluator.snippets_access_level, snippets_access_level: evaluator.snippets_access_level,
issues_access_level: evaluator.issues_access_level, issues_access_level: evaluator.issues_access_level,
merge_requests_access_level: evaluator.merge_requests_access_level, merge_requests_access_level: merge_requests_access_level,
repository_access_level: evaluator.repository_access_level repository_access_level: evaluator.repository_access_level
) )
end end
......
...@@ -66,6 +66,7 @@ describe Gitlab::GitAccess, lib: true do ...@@ -66,6 +66,7 @@ describe Gitlab::GitAccess, lib: true do
context 'pull code' do context 'pull code' do
it { expect(subject.allowed?).to be_falsey } it { expect(subject.allowed?).to be_falsey }
it { expect(subject.message).to match(/You are not allowed to download code/) }
end end
end end
...@@ -77,6 +78,7 @@ describe Gitlab::GitAccess, lib: true do ...@@ -77,6 +78,7 @@ describe Gitlab::GitAccess, lib: true do
context 'pull code' do context 'pull code' do
it { expect(subject.allowed?).to be_falsey } it { expect(subject.allowed?).to be_falsey }
it { expect(subject.message).to match(/Your account has been blocked/) }
end end
end end
...@@ -84,6 +86,29 @@ describe Gitlab::GitAccess, lib: true do ...@@ -84,6 +86,29 @@ describe Gitlab::GitAccess, lib: true do
context 'pull code' do context 'pull code' do
it { expect(subject.allowed?).to be_falsey } it { expect(subject.allowed?).to be_falsey }
end end
context 'when project is public' do
let(:public_project) { create(:project, :public) }
let(:guest_access) { Gitlab::GitAccess.new(nil, public_project, 'web', authentication_abilities: []) }
subject { guest_access.check('git-upload-pack', '_any') }
context 'when repository is enabled' do
it 'give access to download code' do
public_project.project_feature.update_attribute(:repository_access_level, ProjectFeature::ENABLED)
expect(subject.allowed?).to be_truthy
end
end
context 'when repository is disabled' do
it 'does not give access to download code' do
public_project.project_feature.update_attribute(:repository_access_level, ProjectFeature::DISABLED)
expect(subject.allowed?).to be_falsey
expect(subject.message).to match(/You are not allowed to download code/)
end
end
end
end end
describe 'deploy key permissions' do describe 'deploy key permissions' do
......
...@@ -18,7 +18,7 @@ describe Gitlab::GitAccessWiki, lib: true do ...@@ -18,7 +18,7 @@ describe Gitlab::GitAccessWiki, lib: true do
project.team << [user, :developer] project.team << [user, :developer]
end end
subject { access.push_access_check(changes) } subject { access.check('git-receive-pack', changes) }
it { expect(subject.allowed?).to be_truthy } it { expect(subject.allowed?).to be_truthy }
end end
......
require 'spec_helper'
describe Guest, lib: true do
let(:public_project) { create(:project, :public) }
let(:private_project) { create(:project, :private) }
let(:internal_project) { create(:project, :internal) }
describe '.can_pull?' do
context 'when project is private' do
it 'does not allow to pull the repo' do
expect(Guest.can?(:download_code, private_project)).to eq(false)
end
end
context 'when project is internal' do
it 'does not allow to pull the repo' do
expect(Guest.can?(:download_code, internal_project)).to eq(false)
end
end
context 'when project is public' do
context 'when repository is disabled' do
it 'does not allow to pull the repo' do
public_project.project_feature.update_attribute(:repository_access_level, ProjectFeature::DISABLED)
expect(Guest.can?(:download_code, public_project)).to eq(false)
end
end
context 'when repository is accessible only by team members' do
it 'does not allow to pull the repo' do
public_project.project_feature.update_attribute(:repository_access_level, ProjectFeature::PRIVATE)
expect(Guest.can?(:download_code, public_project)).to eq(false)
end
end
context 'when repository is enabled' do
it 'allows to pull the repo' do
public_project.project_feature.update_attribute(:repository_access_level, ProjectFeature::ENABLED)
expect(Guest.can?(:download_code, public_project)).to eq(true)
end
end
end
end
end
...@@ -115,6 +115,38 @@ describe 'Git HTTP requests', lib: true do ...@@ -115,6 +115,38 @@ describe 'Git HTTP requests', lib: true do
end.to raise_error(JWT::DecodeError) end.to raise_error(JWT::DecodeError)
end end
end end
context 'when the repo is public' do
context 'but the repo is disabled' do
it 'does not allow to clone the repo' do
project = create(:project, :public, repository_access_level: ProjectFeature::DISABLED)
download("#{project.path_with_namespace}.git", {}) do |response|
expect(response).to have_http_status(:unauthorized)
end
end
end
context 'but the repo is enabled' do
it 'allows to clone the repo' do
project = create(:project, :public, repository_access_level: ProjectFeature::ENABLED)
download("#{project.path_with_namespace}.git", {}) do |response|
expect(response).to have_http_status(:ok)
end
end
end
context 'but only project members are allowed' do
it 'does not allow to clone the repo' do
project = create(:project, :public, repository_access_level: ProjectFeature::PRIVATE)
download("#{project.path_with_namespace}.git", {}) do |response|
expect(response).to have_http_status(:unauthorized)
end
end
end
end
end end
context "when the project is private" do context "when the project is private" do
......
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