diff options
-rw-r--r-- | lib/gitlab/auth/auth_finders.rb | 4 | ||||
-rw-r--r-- | lib/gitlab/auth/request_authenticator.rb | 24 | ||||
-rw-r--r-- | spec/lib/gitlab/auth/request_authenticator_spec.rb | 70 | ||||
-rw-r--r-- | spec/requests/rack_attack_global_spec.rb | 45 | ||||
-rw-r--r-- | workhorse/internal/artifacts/entry.go | 3 |
5 files changed, 119 insertions, 27 deletions
diff --git a/lib/gitlab/auth/auth_finders.rb b/lib/gitlab/auth/auth_finders.rb index f6ee08defcf..9c33a5fc872 100644 --- a/lib/gitlab/auth/auth_finders.rb +++ b/lib/gitlab/auth/auth_finders.rb @@ -342,6 +342,10 @@ module Gitlab Gitlab::PathRegex.repository_git_lfs_route_regex.match?(current_request.path) end + def git_or_lfs_request? + git_request? || git_lfs_request? + end + def archive_request? current_request.path.include?('/-/archive/') end diff --git a/lib/gitlab/auth/request_authenticator.rb b/lib/gitlab/auth/request_authenticator.rb index dfc682e8a5c..08214bbd449 100644 --- a/lib/gitlab/auth/request_authenticator.rb +++ b/lib/gitlab/auth/request_authenticator.rb @@ -35,13 +35,31 @@ module Gitlab find_user_from_static_object_token(request_format) || find_user_from_basic_auth_job || find_user_from_job_token || - find_user_from_lfs_token || - find_user_from_personal_access_token || - find_user_from_basic_auth_password + find_user_from_personal_access_token_for_api_or_git || + find_user_for_git_or_lfs_request rescue Gitlab::Auth::AuthenticationError nil end + # To prevent Rack Attack from incorrectly rate limiting + # authenticated Git activity, we need to authenticate the user + # from other means (e.g. HTTP Basic Authentication) only if the + # request originated from a Git or Git LFS + # request. Repositories::GitHttpClientController or + # Repositories::LfsApiController normally does the authentication, + # but Rack Attack runs before those controllers. + def find_user_for_git_or_lfs_request + return unless git_or_lfs_request? + + find_user_from_lfs_token || find_user_from_basic_auth_password + end + + def find_user_from_personal_access_token_for_api_or_git + return unless api_request? || git_or_lfs_request? + + find_user_from_personal_access_token + end + def valid_access_token?(scopes: []) validate_access_token!(scopes: scopes) diff --git a/spec/lib/gitlab/auth/request_authenticator_spec.rb b/spec/lib/gitlab/auth/request_authenticator_spec.rb index 28e93a8da52..2543eb3a5e9 100644 --- a/spec/lib/gitlab/auth/request_authenticator_spec.rb +++ b/spec/lib/gitlab/auth/request_authenticator_spec.rb @@ -81,32 +81,72 @@ RSpec.describe Gitlab::Auth::RequestAuthenticator do expect(subject.find_sessionless_user(:api)).to eq job_token_user end - it 'returns lfs_token user if no job_token user found' do - allow_any_instance_of(described_class) - .to receive(:find_user_from_lfs_token) - .and_return(lfs_token_user) - - expect(subject.find_sessionless_user(:api)).to eq lfs_token_user - end - - it 'returns basic_auth_access_token user if no lfs_token user found' do + it 'returns nil even if basic_auth_access_token is available' do allow_any_instance_of(described_class) .to receive(:find_user_from_personal_access_token) .and_return(basic_auth_access_token_user) - expect(subject.find_sessionless_user(:api)).to eq basic_auth_access_token_user + expect(subject.find_sessionless_user(:api)).to be_nil end - it 'returns basic_auth_access_password user if no basic_auth_access_token user found' do + it 'returns nil even if find_user_from_lfs_token is available' do allow_any_instance_of(described_class) - .to receive(:find_user_from_basic_auth_password) - .and_return(basic_auth_password_user) + .to receive(:find_user_from_lfs_token) + .and_return(lfs_token_user) - expect(subject.find_sessionless_user(:api)).to eq basic_auth_password_user + expect(subject.find_sessionless_user(:api)).to be_nil end it 'returns nil if no user found' do - expect(subject.find_sessionless_user(:api)).to be_blank + expect(subject.find_sessionless_user(:api)).to be_nil + end + + context 'in an API request' do + before do + env['SCRIPT_NAME'] = '/api/v4/projects' + end + + it 'returns basic_auth_access_token user if no job_token_user found' do + allow_any_instance_of(described_class) + .to receive(:find_user_from_personal_access_token) + .and_return(basic_auth_access_token_user) + + expect(subject.find_sessionless_user(:api)).to eq basic_auth_access_token_user + end + end + + context 'in a Git request' do + before do + env['SCRIPT_NAME'] = '/group/project.git/info/refs' + end + + it 'returns lfs_token user if no job_token user found' do + allow_any_instance_of(described_class) + .to receive(:find_user_from_lfs_token) + .and_return(lfs_token_user) + + expect(subject.find_sessionless_user(nil)).to eq lfs_token_user + end + + it 'returns basic_auth_access_token user if no lfs_token user found' do + allow_any_instance_of(described_class) + .to receive(:find_user_from_personal_access_token) + .and_return(basic_auth_access_token_user) + + expect(subject.find_sessionless_user(nil)).to eq basic_auth_access_token_user + end + + it 'returns basic_auth_access_password user if no basic_auth_access_token user found' do + allow_any_instance_of(described_class) + .to receive(:find_user_from_basic_auth_password) + .and_return(basic_auth_password_user) + + expect(subject.find_sessionless_user(nil)).to eq basic_auth_password_user + end + + it 'returns nil if no user found' do + expect(subject.find_sessionless_user(nil)).to be_blank + end end it 'rescue Gitlab::Auth::AuthenticationError exceptions' do diff --git a/spec/requests/rack_attack_global_spec.rb b/spec/requests/rack_attack_global_spec.rb index 87ef6fa1a18..be942f6ae86 100644 --- a/spec/requests/rack_attack_global_spec.rb +++ b/spec/requests/rack_attack_global_spec.rb @@ -933,17 +933,28 @@ RSpec.describe 'Rack Attack global throttles', :use_clean_rails_memory_store_cac end context 'authenticated with lfs token' do - it 'request is authenticated by token in basic auth' do - lfs_token = Gitlab::LfsToken.new(user) - encoded_login = ["#{user.username}:#{lfs_token.token}"].pack('m0') + let(:lfs_url) { '/namespace/repo.git/info/lfs/objects/batch' } + let(:lfs_token) { Gitlab::LfsToken.new(user) } + let(:encoded_login) { ["#{user.username}:#{lfs_token.token}"].pack('m0') } + let(:headers) { { 'AUTHORIZATION' => "Basic #{encoded_login}" } } + it 'request is authenticated by token in basic auth' do expect_authenticated_request - get url, headers: { 'AUTHORIZATION' => "Basic #{encoded_login}" } + get lfs_url, headers: headers + end + + it 'request is not authenticated with API URL' do + expect_unauthenticated_request + + get url, headers: headers end end context 'authenticated with regular login' do + let(:encoded_login) { ["#{user.username}:#{user.password}"].pack('m0') } + let(:headers) { { 'AUTHORIZATION' => "Basic #{encoded_login}" } } + it 'request is authenticated after login' do login_as(user) @@ -952,12 +963,30 @@ RSpec.describe 'Rack Attack global throttles', :use_clean_rails_memory_store_cac get url end - it 'request is authenticated by credentials in basic auth' do - encoded_login = ["#{user.username}:#{user.password}"].pack('m0') + it 'request is not authenticated by credentials in basic auth' do + expect_unauthenticated_request - expect_authenticated_request + get url, headers: headers + end + + context 'with POST git-upload-pack' do + it 'request is authenticated by credentials in basic auth' do + expect(::Gitlab::Workhorse).to receive(:verify_api_request!) + + expect_authenticated_request - get url, headers: { 'AUTHORIZATION' => "Basic #{encoded_login}" } + post '/namespace/repo.git/git-upload-pack', headers: headers + end + end + + context 'with GET info/refs' do + it 'request is authenticated by credentials in basic auth' do + expect(::Gitlab::Workhorse).to receive(:verify_api_request!) + + expect_authenticated_request + + get '/namespace/repo.git/info/refs?service=git-upload-pack', headers: headers + end end end end diff --git a/workhorse/internal/artifacts/entry.go b/workhorse/internal/artifacts/entry.go index 91a0843f1ee..d5b3dfb672c 100644 --- a/workhorse/internal/artifacts/entry.go +++ b/workhorse/internal/artifacts/entry.go @@ -14,6 +14,7 @@ import ( "syscall" "gitlab.com/gitlab-org/labkit/log" + "gitlab.com/gitlab-org/labkit/mask" "gitlab.com/gitlab-org/gitlab/workhorse/internal/helper" "gitlab.com/gitlab-org/gitlab/workhorse/internal/senddata" @@ -35,7 +36,7 @@ func (e *entry) Inject(w http.ResponseWriter, r *http.Request, sendData string) log.WithContextFields(r.Context(), log.Fields{ "entry": params.Entry, - "archive": params.Archive, + "archive": mask.URL(params.Archive), "path": r.URL.Path, }).Print("SendEntry: sending") |