diff options
author | Dmitriy Zaporozhets <dzaporozhets@gitlab.com> | 2015-03-03 20:05:12 +0000 |
---|---|---|
committer | Dmitriy Zaporozhets <dzaporozhets@gitlab.com> | 2015-03-03 20:05:12 +0000 |
commit | 8c47a72a4ed3df2327104e029307b5d804525886 (patch) | |
tree | 37070b999e2aa5dc6cbfdf14209575716d9e86af | |
parent | a7fad44bd361c68c6f4ff0fbeb5ad067ef2b74b1 (diff) | |
parent | 0e11be40c39df66859ae0f3dc265cd903820c153 (diff) | |
download | gitlab-ce-8c47a72a4ed3df2327104e029307b5d804525886.tar.gz |
Merge branch 'project-existence-leak' into 'master'
Don't leak information about private project existence via Git-over-SSH/HTTP.
Fixes #2040 and https://gitlab.com/gitlab-org/gitlab-ce/issues/343.
Both `Grack::Auth` (used by Git-over-HTTP) and `Api::Internal /allowed` (used by gitlab-shell/Git-over-SSH) now return a generic "Not Found" error when the project exists but the user doesn't have access to it.
See merge request !1578
-rw-r--r-- | lib/api/internal.rb | 39 | ||||
-rw-r--r-- | lib/gitlab/backend/grack_auth.rb | 52 | ||||
-rw-r--r-- | spec/lib/gitlab/backend/grack_auth_spec.rb | 146 |
3 files changed, 196 insertions, 41 deletions
diff --git a/lib/api/internal.rb b/lib/api/internal.rb index ba3fe619b92..753d0fcbd98 100644 --- a/lib/api/internal.rb +++ b/lib/api/internal.rb @@ -16,6 +16,17 @@ module API # post "/allowed" do status 200 + + actor = if params[:key_id] + Key.find_by(id: params[:key_id]) + elsif params[:user_id] + User.find_by(id: params[:user_id]) + end + + unless actor + return Gitlab::GitAccessStatus.new(false, 'No such user or key') + end + project_path = params[:project] # Check for *.wiki repositories. @@ -32,26 +43,20 @@ module API project = Project.find_with_namespace(project_path) - unless project - return Gitlab::GitAccessStatus.new(false, 'No such project') + if project + status = access.check( + actor, + params[:action], + project, + params[:changes] + ) end - actor = if params[:key_id] - Key.find_by(id: params[:key_id]) - elsif params[:user_id] - User.find_by(id: params[:user_id]) - end - - unless actor - return Gitlab::GitAccessStatus.new(false, 'No such user or key') + if project && status && status.allowed? + status + else + Gitlab::GitAccessStatus.new(false, 'No such project') end - - access.check( - actor, - params[:action], - project, - params[:changes] - ) end # diff --git a/lib/gitlab/backend/grack_auth.rb b/lib/gitlab/backend/grack_auth.rb index dc4b945f9d4..ee877e099b1 100644 --- a/lib/gitlab/backend/grack_auth.rb +++ b/lib/gitlab/backend/grack_auth.rb @@ -10,8 +10,9 @@ module Grack @request = Rack::Request.new(env) @auth = Request.new(env) - # Need this patch due to the rails mount + @gitlab_ci = false + # Need this patch due to the rails mount # Need this if under RELATIVE_URL_ROOT unless Gitlab.config.gitlab.relative_url_root.empty? # If website is mounted using relative_url_root need to remove it first @@ -22,8 +23,12 @@ module Grack @env['SCRIPT_NAME'] = "" - if project - auth! + auth! + + if project && authorized_request? + @app.call(env) + elsif @user.nil? && !@gitlab_ci + unauthorized else render_not_found end @@ -32,35 +37,30 @@ module Grack private def auth! - if @auth.provided? - return bad_request unless @auth.basic? - - # Authentication with username and password - login, password = @auth.credentials + return unless @auth.provided? - # Allow authentication for GitLab CI service - # if valid token passed - if gitlab_ci_request?(login, password) - return @app.call(env) - end + return bad_request unless @auth.basic? - @user = authenticate_user(login, password) + # Authentication with username and password + login, password = @auth.credentials - if @user - Gitlab::ShellEnv.set_env(@user) - @env['REMOTE_USER'] = @auth.username - end + # Allow authentication for GitLab CI service + # if valid token passed + if gitlab_ci_request?(login, password) + @gitlab_ci = true + return end - if authorized_request? - @app.call(env) - else - unauthorized + @user = authenticate_user(login, password) + + if @user + Gitlab::ShellEnv.set_env(@user) + @env['REMOTE_USER'] = @auth.username end end def gitlab_ci_request?(login, password) - if login == "gitlab-ci-token" && project.gitlab_ci? + if login == "gitlab-ci-token" && project && project.gitlab_ci? token = project.gitlab_ci_service.token if token.present? && token == password && git_cmd == 'git-upload-pack' @@ -107,6 +107,8 @@ module Grack end def authorized_request? + return true if @gitlab_ci + case git_cmd when *Gitlab::GitAccess::DOWNLOAD_COMMANDS if user @@ -141,7 +143,9 @@ module Grack end def project - @project ||= project_by_path(@request.path_info) + return @project if defined?(@project) + + @project = project_by_path(@request.path_info) end def project_by_path(path) diff --git a/spec/lib/gitlab/backend/grack_auth_spec.rb b/spec/lib/gitlab/backend/grack_auth_spec.rb new file mode 100644 index 00000000000..768312f0028 --- /dev/null +++ b/spec/lib/gitlab/backend/grack_auth_spec.rb @@ -0,0 +1,146 @@ +require "spec_helper" + +describe Grack::Auth do + let(:user) { create(:user) } + let(:project) { create(:project) } + + let(:app) { lambda { |env| [200, {}, "Success!"] } } + let!(:auth) { Grack::Auth.new(app) } + let(:env) { + { + "rack.input" => "", + "REQUEST_METHOD" => "GET", + "QUERY_STRING" => "service=git-upload-pack" + } + } + let(:status) { auth.call(env).first } + + describe "#call" do + context "when the project doesn't exist" do + before do + env["PATH_INFO"] = "doesnt/exist.git" + end + + context "when no authentication is provided" do + it "responds with status 401" do + expect(status).to eq(401) + end + end + + context "when username and password are provided" do + context "when authentication fails" do + before do + env["HTTP_AUTHORIZATION"] = ActionController::HttpAuthentication::Basic.encode_credentials(user.username, "nope") + end + + it "responds with status 401" do + expect(status).to eq(401) + end + end + + context "when authentication succeeds" do + before do + env["HTTP_AUTHORIZATION"] = ActionController::HttpAuthentication::Basic.encode_credentials(user.username, user.password) + end + + it "responds with status 404" do + expect(status).to eq(404) + end + end + end + end + + context "when the project exists" do + before do + env["PATH_INFO"] = project.path_with_namespace + ".git" + end + + context "when the project is public" do + before do + project.update_attribute(:visibility_level, Project::PUBLIC) + end + + it "responds with status 200" do + expect(status).to eq(200) + end + end + + context "when the project is private" do + before do + project.update_attribute(:visibility_level, Project::PRIVATE) + end + + context "when no authentication is provided" do + it "responds with status 401" do + expect(status).to eq(401) + end + end + + context "when username and password are provided" do + context "when authentication fails" do + before do + env["HTTP_AUTHORIZATION"] = ActionController::HttpAuthentication::Basic.encode_credentials(user.username, "nope") + end + + it "responds with status 401" do + expect(status).to eq(401) + end + end + + context "when authentication succeeds" do + before do + env["HTTP_AUTHORIZATION"] = ActionController::HttpAuthentication::Basic.encode_credentials(user.username, user.password) + end + + context "when the user has access to the project" do + before do + project.team << [user, :master] + end + + context "when the user is blocked" do + before do + user.block + project.team << [user, :master] + end + + it "responds with status 404" do + expect(status).to eq(404) + end + end + + context "when the user isn't blocked" do + it "responds with status 200" do + expect(status).to eq(200) + end + end + end + + context "when the user doesn't have access to the project" do + it "responds with status 404" do + expect(status).to eq(404) + end + end + end + end + + context "when a gitlab ci token is provided" do + let(:token) { "123" } + + before do + gitlab_ci_service = project.build_gitlab_ci_service + gitlab_ci_service.active = true + gitlab_ci_service.token = token + gitlab_ci_service.project_url = "http://google.com" + gitlab_ci_service.save + + env["HTTP_AUTHORIZATION"] = ActionController::HttpAuthentication::Basic.encode_credentials("gitlab-ci-token", token) + end + + it "responds with status 200" do + expect(status).to eq(200) + end + end + end + end + end +end |