From b0bf92140f469db90ef378fd42a6f65eee1d4633 Mon Sep 17 00:00:00 2001
From: Douwe Maan <douwe@gitlab.com>
Date: Wed, 2 Nov 2016 21:50:44 +0000
Subject: [PATCH] Merge branch 'fix-unathorized-cloning' into 'security'
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Ensure external users are not able to clone disabled repositories.
Closes https://gitlab.com/gitlab-org/gitlab-ce/issues/23788
See merge request !2017
Signed-off-by: Rémy Coutable <remy@rymai.me>
---
.../projects/git_http_client_controller.rb | 8 +-
.../projects/git_http_controller.rb | 6 +-
app/helpers/lfs_helper.rb | 4 +-
app/models/guest.rb | 7 ++
...ntainer_registry_authentication_service.rb | 2 +-
lib/gitlab/git_access.rb | 91 ++++++++++++-------
spec/factories/projects.rb | 10 +-
spec/lib/gitlab/git_access_spec.rb | 25 +++++
spec/lib/gitlab/git_access_wiki_spec.rb | 2 +-
spec/models/guest_spec.rb | 47 ++++++++++
spec/requests/git_http_spec.rb | 32 +++++++
11 files changed, 187 insertions(+), 47 deletions(-)
create mode 100644 app/models/guest.rb
create mode 100644 spec/models/guest_spec.rb
diff --git a/app/controllers/projects/git_http_client_controller.rb b/app/controllers/projects/git_http_client_controller.rb
index 383e184d796..3f41916e6d3 100644
--- a/app/controllers/projects/git_http_client_controller.rb
+++ b/app/controllers/projects/git_http_client_controller.rb
@@ -21,10 +21,6 @@ class Projects::GitHttpClientController < Projects::ApplicationController
def authenticate_user
@authentication_result = Gitlab::Auth::Result.new
- if project && project.public? && download_request?
- return # Allow access
- end
-
if allow_basic_auth? && basic_auth_provided?
login, password = user_name_and_password(request)
@@ -41,6 +37,10 @@ class Projects::GitHttpClientController < Projects::ApplicationController
send_final_spnego_response
return # Allow access
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
send_challenges
diff --git a/app/controllers/projects/git_http_controller.rb b/app/controllers/projects/git_http_controller.rb
index 662d38b10a5..13caeb42d40 100644
--- a/app/controllers/projects/git_http_controller.rb
+++ b/app/controllers/projects/git_http_controller.rb
@@ -78,11 +78,7 @@ class Projects::GitHttpController < Projects::GitHttpClientController
def upload_pack_allowed?
return false unless Gitlab.config.gitlab_shell.upload_pack
- if user
- access_check.allowed?
- else
- ci? || project.public?
- end
+ access_check.allowed? || ci?
end
def access
diff --git a/app/helpers/lfs_helper.rb b/app/helpers/lfs_helper.rb
index 95b60aeab5f..d3966ba1f10 100644
--- a/app/helpers/lfs_helper.rb
+++ b/app/helpers/lfs_helper.rb
@@ -1,6 +1,6 @@
module LfsHelper
include Gitlab::Routing.url_helpers
-
+
def require_lfs_enabled!
return if Gitlab.config.lfs.enabled
@@ -27,7 +27,7 @@ module LfsHelper
def lfs_download_access?
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
def user_can_download_code?
diff --git a/app/models/guest.rb b/app/models/guest.rb
new file mode 100644
index 00000000000..01285ca1264
--- /dev/null
+++ b/app/models/guest.rb
@@ -0,0 +1,7 @@
+class Guest
+ class << self
+ def can?(action, subject)
+ Ability.allowed?(nil, action, subject)
+ end
+ end
+end
diff --git a/app/services/auth/container_registry_authentication_service.rb b/app/services/auth/container_registry_authentication_service.rb
index 8ea88da8a53..3fc1c70be75 100644
--- a/app/services/auth/container_registry_authentication_service.rb
+++ b/app/services/auth/container_registry_authentication_service.rb
@@ -76,7 +76,7 @@ module Auth
case requested_action
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'
build_can_push?(requested_project) || user_can_push?(requested_project)
else
diff --git a/lib/gitlab/git_access.rb b/lib/gitlab/git_access.rb
index 799794c0171..bcbf6455998 100644
--- a/lib/gitlab/git_access.rb
+++ b/lib/gitlab/git_access.rb
@@ -2,8 +2,18 @@
# class return an instance of `GitlabAccessStatus`
module Gitlab
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 }
PUSH_COMMANDS = %w{ git-receive-pack }
+ ALL_COMMANDS = DOWNLOAD_COMMANDS + PUSH_COMMANDS
attr_reader :actor, :project, :protocol, :user_access, :authentication_abilities
@@ -16,56 +26,43 @@ module Gitlab
end
def check(cmd, changes)
- return build_status_object(false, "Git access over #{protocol.upcase} is not allowed") unless protocol_allowed?
-
- unless actor
- return build_status_object(false, "No user or key was provided.")
- 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
+ check_protocol!
+ check_active_user!
+ check_project_accessibility!
+ check_command_existence!(cmd)
case cmd
when *DOWNLOAD_COMMANDS
download_access_check
when *PUSH_COMMANDS
push_access_check(changes)
- else
- build_status_object(false, "The command you're trying to execute is not allowed.")
end
+
+ build_status_object(true)
+ rescue UnauthorizedError => ex
+ build_status_object(false, ex.message)
end
def download_access_check
if user
user_download_access_check
- elsif deploy_key
- build_status_object(true)
- else
- raise 'Wrong actor'
+ elsif deploy_key.nil? && !Guest.can?(:download_code, project)
+ raise UnauthorizedError, ERROR_MESSAGES[:download]
end
end
def push_access_check(changes)
if user
user_push_access_check(changes)
- elsif deploy_key
- build_status_object(false, "Deploy keys are not allowed to push code.")
else
- raise 'Wrong actor'
+ raise UnauthorizedError, ERROR_MESSAGES[deploy_key ? :deploy_key : :upload]
end
end
def user_download_access_check
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
-
- build_status_object(true)
end
def user_can_download_code?
@@ -78,15 +75,15 @@ module Gitlab
def user_push_access_check(changes)
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
if changes.blank?
- return build_status_object(true)
+ return # Allow access.
end
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
changes_list = Gitlab::ChangesList.new(changes)
@@ -96,11 +93,9 @@ module Gitlab
status = change_access_check(change)
unless status.allowed?
# If user does not have access to make at least one change - cancel all push
- return status
+ raise UnauthorizedError, status.message
end
end
-
- build_status_object(true)
end
def change_access_check(change)
@@ -113,6 +108,30 @@ module Gitlab
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)
Checks::MatchingMergeRequest.new(newrev, branch_name, project).match?
end
@@ -130,6 +149,16 @@ module Gitlab
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
def user
diff --git a/spec/factories/projects.rb b/spec/factories/projects.rb
index dd4a86b1e31..bfd88a254f1 100644
--- a/spec/factories/projects.rb
+++ b/spec/factories/projects.rb
@@ -49,13 +49,17 @@ FactoryGirl.define do
end
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.
- update_attributes(
+ update_attributes!(
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,
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
)
end
diff --git a/spec/lib/gitlab/git_access_spec.rb b/spec/lib/gitlab/git_access_spec.rb
index 62aa212f1f6..f1d0a190002 100644
--- a/spec/lib/gitlab/git_access_spec.rb
+++ b/spec/lib/gitlab/git_access_spec.rb
@@ -66,6 +66,7 @@ describe Gitlab::GitAccess, lib: true do
context 'pull code' do
it { expect(subject.allowed?).to be_falsey }
+ it { expect(subject.message).to match(/You are not allowed to download code/) }
end
end
@@ -77,6 +78,7 @@ describe Gitlab::GitAccess, lib: true do
context 'pull code' do
it { expect(subject.allowed?).to be_falsey }
+ it { expect(subject.message).to match(/Your account has been blocked/) }
end
end
@@ -84,6 +86,29 @@ describe Gitlab::GitAccess, lib: true do
context 'pull code' do
it { expect(subject.allowed?).to be_falsey }
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
describe 'deploy key permissions' do
diff --git a/spec/lib/gitlab/git_access_wiki_spec.rb b/spec/lib/gitlab/git_access_wiki_spec.rb
index 576cda595bb..576aa5c366f 100644
--- a/spec/lib/gitlab/git_access_wiki_spec.rb
+++ b/spec/lib/gitlab/git_access_wiki_spec.rb
@@ -18,7 +18,7 @@ describe Gitlab::GitAccessWiki, lib: true do
project.team << [user, :developer]
end
- subject { access.push_access_check(changes) }
+ subject { access.check('git-receive-pack', changes) }
it { expect(subject.allowed?).to be_truthy }
end
diff --git a/spec/models/guest_spec.rb b/spec/models/guest_spec.rb
new file mode 100644
index 00000000000..d79f929f7a1
--- /dev/null
+++ b/spec/models/guest_spec.rb
@@ -0,0 +1,47 @@
+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
diff --git a/spec/requests/git_http_spec.rb b/spec/requests/git_http_spec.rb
index 27f0fd22ae6..f1728d61def 100644
--- a/spec/requests/git_http_spec.rb
+++ b/spec/requests/git_http_spec.rb
@@ -115,6 +115,38 @@ describe 'Git HTTP requests', lib: true do
end.to raise_error(JWT::DecodeError)
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
context "when the project is private" do
--
GitLab