diff options
| -rw-r--r-- | app/controllers/sessions_controller.rb | 10 | ||||
| -rw-r--r-- | app/models/application_setting.rb | 3 | ||||
| -rw-r--r-- | config/application.rb | 5 | ||||
| -rw-r--r-- | config/initializers/doorkeeper.rb | 6 | ||||
| -rw-r--r-- | config/initializers/request_context.rb | 3 | ||||
| -rw-r--r-- | db/migrate/20170210131347_add_unique_ips_limit_to_application_settings.rb | 17 | ||||
| -rw-r--r-- | db/schema.rb | 5 | ||||
| -rw-r--r-- | lib/gitlab/auth.rb | 22 | ||||
| -rw-r--r-- | lib/gitlab/auth/unique_ips_limiter.rb | 70 | ||||
| -rw-r--r-- | lib/gitlab/request_context.rb | 25 | ||||
| -rw-r--r-- | spec/lib/gitlab/auth/unique_ips_limiter_spec.rb | 88 | ||||
| -rw-r--r-- | spec/lib/gitlab/request_context_spec.rb | 40 |
12 files changed, 278 insertions, 16 deletions
diff --git a/app/controllers/sessions_controller.rb b/app/controllers/sessions_controller.rb index 7d81c96262f..3f5b92d9a99 100644 --- a/app/controllers/sessions_controller.rb +++ b/app/controllers/sessions_controller.rb @@ -67,10 +67,12 @@ class SessionsController < Devise::SessionsController end def find_user - if session[:otp_user_id] - User.find(session[:otp_user_id]) - elsif user_params[:login] - User.by_login(user_params[:login]) + Gitlab::Auth::UniqueIpsLimiter.limit_user! do + if session[:otp_user_id] + User.find(session[:otp_user_id]) + elsif user_params[:login] + User.by_login(user_params[:login]) + end end end diff --git a/app/models/application_setting.rb b/app/models/application_setting.rb index 255e8c4ff78..2f64fb1a6a2 100644 --- a/app/models/application_setting.rb +++ b/app/models/application_setting.rb @@ -184,6 +184,9 @@ class ApplicationSetting < ActiveRecord::Base domain_whitelist: Settings.gitlab['domain_whitelist'], gravatar_enabled: Settings.gravatar['enabled'], help_page_text: nil, + unique_ips_limit_per_user: 10, + unique_ips_limit_time_window: 3600, + unique_ips_limit_enabled: false, housekeeping_bitmaps_enabled: true, housekeeping_enabled: true, housekeeping_full_repack_period: 50, diff --git a/config/application.rb b/config/application.rb index f1a986d1731..c4dea9e92b0 100644 --- a/config/application.rb +++ b/config/application.rb @@ -7,6 +7,9 @@ Bundler.require(:default, Rails.env) module Gitlab class Application < Rails::Application require_dependency Rails.root.join('lib/gitlab/redis') + require_dependency Rails.root.join('lib/gitlab/request_context') + require_dependency Rails.root.join('lib/gitlab/auth') + require_dependency Rails.root.join('lib/gitlab/auth/unique_ips_limiter') # Settings in config/environments/* take precedence over those specified here. # Application configuration should go into files in config/initializers @@ -111,6 +114,8 @@ module Gitlab config.middleware.insert_before Warden::Manager, Rack::Attack + config.middleware.insert_before Warden::Manager, Gitlab::Auth::UniqueIpsLimiter + # Allow access to GitLab API from other domains config.middleware.insert_before Warden::Manager, Rack::Cors do allow do diff --git a/config/initializers/doorkeeper.rb b/config/initializers/doorkeeper.rb index 88cd0f5f652..44b658e5872 100644 --- a/config/initializers/doorkeeper.rb +++ b/config/initializers/doorkeeper.rb @@ -12,8 +12,10 @@ Doorkeeper.configure do end resource_owner_from_credentials do |routes| - user = Gitlab::Auth.find_with_user_password(params[:username], params[:password]) - user unless user.try(:two_factor_enabled?) + Gitlab::Auth::UniqueIpsLimiter.limit_user! do + user = Gitlab::Auth.find_with_user_password(params[:username], params[:password]) + user unless user.try(:two_factor_enabled?) + end end # If you want to restrict access to the web interface for adding oauth authorized applications, you need to declare the block below. diff --git a/config/initializers/request_context.rb b/config/initializers/request_context.rb new file mode 100644 index 00000000000..0b485fc1adc --- /dev/null +++ b/config/initializers/request_context.rb @@ -0,0 +1,3 @@ +Rails.application.configure do |config| + config.middleware.insert_after RequestStore::Middleware, Gitlab::RequestContext +end diff --git a/db/migrate/20170210131347_add_unique_ips_limit_to_application_settings.rb b/db/migrate/20170210131347_add_unique_ips_limit_to_application_settings.rb new file mode 100644 index 00000000000..2aa305f6b58 --- /dev/null +++ b/db/migrate/20170210131347_add_unique_ips_limit_to_application_settings.rb @@ -0,0 +1,17 @@ +class AddUniqueIpsLimitToApplicationSettings < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + DOWNTIME = false + disable_ddl_transaction! + + def up + add_column_with_default(:application_settings, :unique_ips_limit_per_user, :integer, default: 10) + add_column_with_default(:application_settings, :unique_ips_limit_time_window, :integer, default: 3600) + add_column_with_default(:application_settings, :unique_ips_limit_enabled, :boolean, default: false) + end + + def down + remove_column(:application_settings, :unique_ips_limit_per_user) + remove_column(:application_settings, :unique_ips_limit_time_window) + remove_column(:application_settings, :unique_ips_limit_enabled) + end +end diff --git a/db/schema.rb b/db/schema.rb index 9deed46530e..8aed4e13b28 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -112,6 +112,9 @@ ActiveRecord::Schema.define(version: 20170305203726) do t.integer "max_pages_size", default: 100, null: false t.integer "terminal_max_session_time", default: 0, null: false t.string "default_artifacts_expire_in", default: "0", null: false + t.integer "unique_ips_limit_per_user", default: 10, null: false + t.integer "unique_ips_limit_time_window", default: 3600, null: false + t.boolean "unique_ips_limit_enabled", default: false, null: false end create_table "audit_events", force: :cascade do |t| @@ -252,8 +255,8 @@ ActiveRecord::Schema.define(version: 20170305203726) do t.integer "lock_version" end - add_index "ci_commits", ["gl_project_id", "ref", "status"], name: "index_ci_commits_on_gl_project_id_and_ref_and_status", using: :btree add_index "ci_commits", ["gl_project_id", "sha"], name: "index_ci_commits_on_gl_project_id_and_sha", using: :btree + add_index "ci_commits", ["gl_project_id", "status"], name: "index_ci_commits_on_gl_project_id_and_status", using: :btree add_index "ci_commits", ["gl_project_id"], name: "index_ci_commits_on_gl_project_id", using: :btree add_index "ci_commits", ["status"], name: "index_ci_commits_on_status", using: :btree add_index "ci_commits", ["user_id"], name: "index_ci_commits_on_user_id", using: :btree diff --git a/lib/gitlab/auth.rb b/lib/gitlab/auth.rb index 0a5abc92190..be055080853 100644 --- a/lib/gitlab/auth.rb +++ b/lib/gitlab/auth.rb @@ -22,23 +22,27 @@ module Gitlab user_with_password_for_git(login, password) || Gitlab::Auth::Result.new + Gitlab::Auth::UniqueIpsLimiter.limit_user! { result.actor } + rate_limit!(ip, success: result.success?, login: login) result end def find_with_user_password(login, password) - user = User.by_login(login) + Gitlab::Auth::UniqueIpsLimiter.limit_user! do + user = User.by_login(login) - # If no user is found, or it's an LDAP server, try LDAP. - # LDAP users are only authenticated via LDAP - if user.nil? || user.ldap_user? - # Second chance - try LDAP authentication - return nil unless Gitlab::LDAP::Config.enabled? + # If no user is found, or it's an LDAP server, try LDAP. + # LDAP users are only authenticated via LDAP + if user.nil? || user.ldap_user? + # Second chance - try LDAP authentication + return nil unless Gitlab::LDAP::Config.enabled? - Gitlab::LDAP::Authentication.login(login, password) - else - user if user.valid_password?(password) + Gitlab::LDAP::Authentication.login(login, password) + else + user if user.valid_password?(password) + end end end diff --git a/lib/gitlab/auth/unique_ips_limiter.rb b/lib/gitlab/auth/unique_ips_limiter.rb new file mode 100644 index 00000000000..21307eb35e4 --- /dev/null +++ b/lib/gitlab/auth/unique_ips_limiter.rb @@ -0,0 +1,70 @@ +module Gitlab + module Auth + class TooManyIps < StandardError + attr_reader :user_id, :ip, :unique_ips_count + + def initialize(user_id, ip, unique_ips_count) + @user_id = user_id + @ip = ip + @unique_ips_count = unique_ips_count + end + + def message + "User #{user_id} from IP: #{ip} tried logging from too many ips: #{unique_ips_count}" + end + end + + class UniqueIpsLimiter + USER_UNIQUE_IPS_PREFIX = 'user_unique_ips' + + class << self + def limit_user_id!(user_id) + if config.unique_ips_limit_enabled + ip = RequestContext.client_ip + unique_ips = count_unique_ips(user_id, ip) + raise TooManyIps.new(user_id, ip, unique_ips) if unique_ips > config.unique_ips_limit_per_user + end + end + + def limit_user!(user = nil) + user = yield if user.nil? + limit_user_id!(user.id) unless user.nil? + user + end + + def config + Gitlab::CurrentSettings.current_application_settings + end + + def count_unique_ips(user_id, ip) + time = Time.now.to_i + key = "#{USER_UNIQUE_IPS_PREFIX}:#{user_id}" + + Gitlab::Redis.with do |redis| + unique_ips_count = nil + redis.multi do |r| + r.zadd(key, time, ip) + r.zremrangebyscore(key, 0, time - config.unique_ips_limit_time_window) + unique_ips_count = r.zcard(key) + end + unique_ips_count.value + end + end + end + + def initialize(app) + @app = app + end + + def call(env) + begin + @app.call(env) + rescue TooManyIps => ex + + Rails.logger.info ex.message + [429, {'Content-Type' => 'text/plain', 'Retry-After' => UniqueIpsLimiter.config.unique_ips_limit_time_window }, ["Retry later\n"]] + end + end + end + end +end diff --git a/lib/gitlab/request_context.rb b/lib/gitlab/request_context.rb new file mode 100644 index 00000000000..5daf04dc92b --- /dev/null +++ b/lib/gitlab/request_context.rb @@ -0,0 +1,25 @@ +module Gitlab + class RequestStoreNotActive < StandardError + end + + class RequestContext + class << self + def client_ip + RequestStore[:client_ip] + end + end + + def initialize(app) + @app = app + end + + def call(env) + raise RequestStoreNotActive.new unless RequestStore.active? + req = Rack::Request.new(env) + + RequestStore[:client_ip] = req.ip + + @app.call(env) + end + end +end
\ No newline at end of file diff --git a/spec/lib/gitlab/auth/unique_ips_limiter_spec.rb b/spec/lib/gitlab/auth/unique_ips_limiter_spec.rb new file mode 100644 index 00000000000..8e9fea0724a --- /dev/null +++ b/spec/lib/gitlab/auth/unique_ips_limiter_spec.rb @@ -0,0 +1,88 @@ +require 'spec_helper' + +describe Gitlab::Auth::UniqueIpsLimiter, lib: true do + let(:user) { create(:user) } + + before(:each) do + Gitlab::Redis.with do |redis| + redis.del("user_unique_ips:#{user.id}") + end + end + + describe '#count_unique_ips' do + + context 'non unique IPs' do + it 'properly counts them' do + expect(Gitlab::Auth::UniqueIpsLimiter.count_unique_ips(user.id, '192.168.1.1')).to eq(1) + expect(Gitlab::Auth::UniqueIpsLimiter.count_unique_ips(user.id, '192.168.1.1')).to eq(1) + end + end + + context 'unique IPs' do + it 'properly counts them' do + expect(Gitlab::Auth::UniqueIpsLimiter.count_unique_ips(user.id, '192.168.1.2')).to eq(1) + expect(Gitlab::Auth::UniqueIpsLimiter.count_unique_ips(user.id, '192.168.1.3')).to eq(2) + end + end + + it 'resets count after specified time window' do + cur_time = Time.now.to_i + allow(Time).to receive(:now).and_return(cur_time) + + expect(Gitlab::Auth::UniqueIpsLimiter.count_unique_ips(user.id, '192.168.1.2')).to eq(1) + expect(Gitlab::Auth::UniqueIpsLimiter.count_unique_ips(user.id, '192.168.1.3')).to eq(2) + + allow(Time).to receive(:now).and_return(cur_time + Gitlab::Auth::UniqueIpsLimiter.config.unique_ips_limit_time_window) + + expect(Gitlab::Auth::UniqueIpsLimiter.count_unique_ips(user.id, '192.168.1.4')).to eq(1) + expect(Gitlab::Auth::UniqueIpsLimiter.count_unique_ips(user.id, '192.168.1.5')).to eq(2) + end + end + + + describe '#limit_user!' do + context 'when unique ips limit is enabled' do + before do + allow(Gitlab::Auth::UniqueIpsLimiter).to receive_message_chain(:config, :unique_ips_limit_enabled).and_return(true) + allow(Gitlab::Auth::UniqueIpsLimiter).to receive_message_chain(:config, :unique_ips_limit_time_window).and_return(10) + end + + context 'when ip limit is set to 1' do + before do + allow(Gitlab::Auth::UniqueIpsLimiter).to receive_message_chain(:config, :unique_ips_limit_per_user).and_return(1) + end + + it 'blocks user trying to login from second ip' do + RequestStore[:client_ip] = '192.168.1.1' + expect(Gitlab::Auth::UniqueIpsLimiter.limit_user! { user }).to eq(user) + + RequestStore[:client_ip] = '192.168.1.2' + expect { Gitlab::Auth::UniqueIpsLimiter.limit_user! { user } }.to raise_error(Gitlab::Auth::TooManyIps) + end + + it 'allows user trying to login from the same ip twice' do + RequestStore[:client_ip] = '192.168.1.1' + expect(Gitlab::Auth::UniqueIpsLimiter.limit_user! { user }).to eq(user) + expect(Gitlab::Auth::UniqueIpsLimiter.limit_user! { user }).to eq(user) + end + end + + context 'when ip limit is set to 2' do + before do + allow(Gitlab::Auth::UniqueIpsLimiter).to receive_message_chain(:config, :unique_ips_limit_per_user).and_return(2) + end + + it 'blocks user trying to login from third ip' do + RequestStore[:client_ip] = '192.168.1.1' + expect(Gitlab::Auth::UniqueIpsLimiter.limit_user! { user }).to eq(user) + + RequestStore[:client_ip] = '192.168.1.2' + expect(Gitlab::Auth::UniqueIpsLimiter.limit_user! { user }).to eq(user) + + RequestStore[:client_ip] = '192.168.1.3' + expect { Gitlab::Auth::UniqueIpsLimiter.limit_user! { user } }.to raise_error(Gitlab::Auth::TooManyIps) + end + end + end + end +end diff --git a/spec/lib/gitlab/request_context_spec.rb b/spec/lib/gitlab/request_context_spec.rb new file mode 100644 index 00000000000..3565fab6ded --- /dev/null +++ b/spec/lib/gitlab/request_context_spec.rb @@ -0,0 +1,40 @@ +require 'spec_helper' + +describe Gitlab::RequestContext, lib: true do + describe '#client_ip' do + subject { Gitlab::RequestContext.client_ip } + let(:app) { -> env {} } + let(:env) { Hash.new } + + context 'when RequestStore::Middleware is used' do + around(:each) do |example| + RequestStore::Middleware.new(-> env { example.run }).call({}) + end + + context 'request' do + let(:ip) { '192.168.1.11' } + + before do + allow_any_instance_of(Rack::Request).to receive(:ip).and_return(ip) + Gitlab::RequestContext.new(app).call(env) + end + + it { is_expected.to eq(ip) } + end + + context 'before RequestContext mw run' do + it { is_expected.to be_nil } + end + end + + context 'RequestStore is not active' do + it { is_expected.to be_nil } + + context 'when RequestContext mw is run' do + subject { -> { Gitlab::RequestContext.new(app).call(env) } } + + it { is_expected.to raise_error(Gitlab::RequestStoreNotActive) } + end + end + end +end |