Commit 7337e578 authored by Stan Hu's avatar Stan Hu

Fix deploy tokens erroneously triggering unique IP limits

Some users were complaining that when the user unique IP limiter was
enabled, they would be banned for some unknown
reason. `AuthFinder.find_for_git_client` can authenticate users from a
multitude of tokens (CI, LFS, HTTP basic auth, etc.), but project deploy
tokens are unique in that they aren't attributed to a specific user. As
a result, if project deploy tokens were used, users that had the same
database ID as a deploy token would erroneously be attributed to using
the IP accessed by the token.

To fix this issue, we only call `Gitlab::Auth::UniqueIpsLimiter` if a
user is returned from the authentication search. Project deploy tokens
could be used from many different IPs, so it doesn't make sense to group
them with user activity.

Possibly fixes https://gitlab.com/gitlab-org/gitlab/issues/22854
parent 15a86e9d
---
title: Fix deploy tokens erroneously triggering unique IP limits
merge_request: 22445
author:
type: fixed
...@@ -54,7 +54,7 @@ module Gitlab ...@@ -54,7 +54,7 @@ module Gitlab
Gitlab::Auth::Result.new Gitlab::Auth::Result.new
rate_limit!(rate_limiter, success: result.success?, login: login) rate_limit!(rate_limiter, success: result.success?, login: login)
Gitlab::Auth::UniqueIpsLimiter.limit_user!(result.actor) look_to_limit_user(result.actor)
return result if result.success? || authenticate_using_internal_or_ldap_password? return result if result.success? || authenticate_using_internal_or_ldap_password?
...@@ -129,6 +129,10 @@ module Gitlab ...@@ -129,6 +129,10 @@ module Gitlab
::Ci::Build::CI_REGISTRY_USER == login ::Ci::Build::CI_REGISTRY_USER == login
end end
def look_to_limit_user(actor)
Gitlab::Auth::UniqueIpsLimiter.limit_user!(actor) if actor.is_a?(User)
end
def authenticate_using_internal_or_ldap_password? def authenticate_using_internal_or_ldap_password?
Gitlab::CurrentSettings.password_authentication_enabled_for_git? || Gitlab::Auth::LDAP::Config.enabled? Gitlab::CurrentSettings.password_authentication_enabled_for_git? || Gitlab::Auth::LDAP::Config.enabled?
end end
......
...@@ -130,6 +130,15 @@ describe Gitlab::Auth, :use_clean_rails_memory_store_caching do ...@@ -130,6 +130,15 @@ describe Gitlab::Auth, :use_clean_rails_memory_store_caching do
gl_auth.find_for_git_client(user.username, user.password, project: nil, ip: 'ip') gl_auth.find_for_git_client(user.username, user.password, project: nil, ip: 'ip')
end end
it 'rate limits a user by unique IPs' do
expect_next_instance_of(Gitlab::Auth::IpRateLimiter) do |rate_limiter|
expect(rate_limiter).to receive(:reset!)
end
expect(Gitlab::Auth::UniqueIpsLimiter).to receive(:limit_user!).twice.and_call_original
gl_auth.find_for_git_client(user.username, user.password, project: nil, ip: 'ip')
end
it 'registers failure for failed auth' do it 'registers failure for failed auth' do
expect_next_instance_of(Gitlab::Auth::IpRateLimiter) do |rate_limiter| expect_next_instance_of(Gitlab::Auth::IpRateLimiter) do |rate_limiter|
expect(rate_limiter).to receive(:register_fail!) expect(rate_limiter).to receive(:register_fail!)
...@@ -415,6 +424,12 @@ describe Gitlab::Auth, :use_clean_rails_memory_store_caching do ...@@ -415,6 +424,12 @@ describe Gitlab::Auth, :use_clean_rails_memory_store_caching do
.to eq(auth_success) .to eq(auth_success)
end end
it 'does not attempt to rate limit unique IPs for a deploy token' do
expect(Gitlab::Auth::UniqueIpsLimiter).not_to receive(:limit_user!)
gl_auth.find_for_git_client(login, deploy_token.token, project: project, ip: 'ip')
end
it 'fails when login is not valid' do it 'fails when login is not valid' do
expect(gl_auth.find_for_git_client('random_login', deploy_token.token, project: project, ip: 'ip')) expect(gl_auth.find_for_git_client('random_login', deploy_token.token, project: project, ip: 'ip'))
.to eq(auth_failure) .to eq(auth_failure)
......
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