diff --git a/app/controllers/admin/application_settings_controller.rb b/app/controllers/admin/application_settings_controller.rb index d807e6263ee..8d831ffdd70 100644 --- a/app/controllers/admin/application_settings_controller.rb +++ b/app/controllers/admin/application_settings_controller.rb @@ -138,6 +138,9 @@ class Admin::ApplicationSettingsController < Admin::ApplicationController :two_factor_grace_period, :user_default_external, :user_oauth_applications, + :unique_ips_limit_per_user, + :unique_ips_limit_time_window, + :unique_ips_limit_enabled, :version_check_enabled, :terminal_max_session_time, diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index cc7b7f247e8..1c66c530cd2 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -40,6 +40,10 @@ class ApplicationController < ActionController::Base render_403 end + rescue_from Gitlab::Auth::TooManyIps do |e| + head :forbidden, retry_after: Gitlab::Auth::UniqueIpsLimiter.config.unique_ips_limit_time_window + end + def redirect_back_or_default(default: root_path, options: {}) redirect_to request.referer.present? ? :back : default, options end diff --git a/app/models/application_setting.rb b/app/models/application_setting.rb index 255e8c4ff78..be632930895 100644 --- a/app/models/application_setting.rb +++ b/app/models/application_setting.rb @@ -64,6 +64,16 @@ class ApplicationSetting < ActiveRecord::Base presence: true, if: :akismet_enabled + validates :unique_ips_limit_per_user, + numericality: { greater_than_or_equal_to: 1 }, + presence: true, + if: :unique_ips_limit_enabled + + validates :unique_ips_limit_time_window, + numericality: { greater_than_or_equal_to: 0 }, + presence: true, + if: :unique_ips_limit_enabled + validates :koding_url, presence: true, if: :koding_enabled @@ -184,6 +194,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/app/views/admin/application_settings/_form.html.haml b/app/views/admin/application_settings/_form.html.haml index 057b584e1bc..00366b0a8c9 100644 --- a/app/views/admin/application_settings/_form.html.haml +++ b/app/views/admin/application_settings/_form.html.haml @@ -360,6 +360,29 @@ Generate API key at %a{ href: 'http://www.akismet.com', target: 'blank' } http://www.akismet.com + .form-group + .col-sm-offset-2.col-sm-10 + .checkbox + = f.label :unique_ips_limit_enabled do + = f.check_box :unique_ips_limit_enabled + Limit sign in from multiple ips + %span.help-block#unique_ip_help_block + Helps prevent malicious users hide their activity + + .form-group + = f.label :unique_ips_limit_per_user, 'IPs per user', class: 'control-label col-sm-2' + .col-sm-10 + = f.number_field :unique_ips_limit_per_user, class: 'form-control' + .help-block + Maximum number of unique IPs per user + + .form-group + = f.label :unique_ips_limit_time_window, 'IP expiration time', class: 'control-label col-sm-2' + .col-sm-10 + = f.number_field :unique_ips_limit_time_window, class: 'form-control' + .help-block + How many seconds an IP will be counted towards the limit + %fieldset %legend Abuse reports .form-group diff --git a/changelogs/unreleased/27520-option-to-prevent-signing-in-from-multiple-ips.yml b/changelogs/unreleased/27520-option-to-prevent-signing-in-from-multiple-ips.yml new file mode 100644 index 00000000000..3050b072863 --- /dev/null +++ b/changelogs/unreleased/27520-option-to-prevent-signing-in-from-multiple-ips.yml @@ -0,0 +1,4 @@ +--- +title: Option to prevent signing in from multiple ips +merge_request: 8998 +author: diff --git a/config/application.rb b/config/application.rb index f1a986d1731..8003c055b27 100644 --- a/config/application.rb +++ b/config/application.rb @@ -7,6 +7,7 @@ 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') # Settings in config/environments/* take precedence over those specified here. # Application configuration should go into files in config/initializers 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/config/initializers/warden.rb b/config/initializers/warden.rb new file mode 100644 index 00000000000..3d83fb92d56 --- /dev/null +++ b/config/initializers/warden.rb @@ -0,0 +1,5 @@ +Rails.application.configure do |config| + Warden::Manager.after_set_user do |user, auth, opts| + Gitlab::Auth::UniqueIpsLimiter.limit_user!(user) + end +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..9ab970134be --- /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 :application_settings, :unique_ips_limit_per_user, :integer + add_column :application_settings, :unique_ips_limit_time_window, :integer + 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..911cb22c8e5 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" + t.integer "unique_ips_limit_time_window" + t.boolean "unique_ips_limit_enabled", default: false, null: false end create_table "audit_events", force: :cascade do |t| diff --git a/lib/api/api.rb b/lib/api/api.rb index 89449ce8813..efbb56ecd2c 100644 --- a/lib/api/api.rb +++ b/lib/api/api.rb @@ -60,6 +60,10 @@ module API error! e.message, e.status, e.headers end + rescue_from Gitlab::Auth::TooManyIps do |e| + rack_response({ 'message' => '403 Forbidden' }.to_json, 403) + end + rescue_from :all do |exception| handle_api_exception(exception) end diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb index a43252a4661..f325f0a3050 100644 --- a/lib/api/helpers.rb +++ b/lib/api/helpers.rb @@ -336,16 +336,17 @@ module API def initial_current_user return @initial_current_user if defined?(@initial_current_user) + Gitlab::Auth::UniqueIpsLimiter.limit_user! do + @initial_current_user ||= find_user_by_private_token(scopes: @scopes) + @initial_current_user ||= doorkeeper_guard(scopes: @scopes) + @initial_current_user ||= find_user_from_warden - @initial_current_user ||= find_user_by_private_token(scopes: @scopes) - @initial_current_user ||= doorkeeper_guard(scopes: @scopes) - @initial_current_user ||= find_user_from_warden + unless @initial_current_user && Gitlab::UserAccess.new(@initial_current_user).allowed? + @initial_current_user = nil + end - unless @initial_current_user && Gitlab::UserAccess.new(@initial_current_user).allowed? - @initial_current_user = nil + @initial_current_user end - - @initial_current_user end def sudo! diff --git a/lib/gitlab/auth.rb b/lib/gitlab/auth.rb index 0a5abc92190..0a0bd0e781c 100644 --- a/lib/gitlab/auth.rb +++ b/lib/gitlab/auth.rb @@ -23,22 +23,25 @@ module Gitlab Gitlab::Auth::Result.new rate_limit!(ip, success: result.success?, login: login) + Gitlab::Auth::UniqueIpsLimiter.limit_user!(result.actor) 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/too_many_ips.rb b/lib/gitlab/auth/too_many_ips.rb new file mode 100644 index 00000000000..ed862791551 --- /dev/null +++ b/lib/gitlab/auth/too_many_ips.rb @@ -0,0 +1,17 @@ +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 + 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..bf2239ca150 --- /dev/null +++ b/lib/gitlab/auth/unique_ips_limiter.rb @@ -0,0 +1,43 @@ +module Gitlab + module Auth + class UniqueIpsLimiter + USER_UNIQUE_IPS_PREFIX = 'user_unique_ips'.freeze + + class << self + def limit_user_id!(user_id) + if config.unique_ips_limit_enabled + ip = RequestContext.client_ip + unique_ips = update_and_return_ips_count(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 block_given? + limit_user_id!(user.id) unless user.nil? + user + end + + def config + Gitlab::CurrentSettings.current_application_settings + end + + def update_and_return_ips_count(user_id, ip) + time = Time.now.utc.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 + end + end +end diff --git a/lib/gitlab/request_context.rb b/lib/gitlab/request_context.rb new file mode 100644 index 00000000000..fef536ecb0b --- /dev/null +++ b/lib/gitlab/request_context.rb @@ -0,0 +1,21 @@ +module Gitlab + class RequestContext + class << self + def client_ip + RequestStore[:client_ip] + end + end + + def initialize(app) + @app = app + end + + def call(env) + req = Rack::Request.new(env) + + RequestStore[:client_ip] = req.ip + + @app.call(env) + end + end +end diff --git a/spec/controllers/sessions_controller_spec.rb b/spec/controllers/sessions_controller_spec.rb index b56c7880b64..a06c29dd91a 100644 --- a/spec/controllers/sessions_controller_spec.rb +++ b/spec/controllers/sessions_controller_spec.rb @@ -25,9 +25,17 @@ describe SessionsController do expect(subject.current_user). to eq user end - it "creates an audit log record" do + it 'creates an audit log record' do expect { post(:create, user: { login: user.username, password: user.password }) }.to change { SecurityEvent.count }.by(1) - expect(SecurityEvent.last.details[:with]).to eq("standard") + expect(SecurityEvent.last.details[:with]).to eq('standard') + end + + include_examples 'user login request with unique ip limit', 302 do + def request + post(:create, user: { login: user.username, password: user.password }) + expect(subject.current_user).to eq user + subject.sign_out user + end end end end 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..94dcddcc30c --- /dev/null +++ b/spec/lib/gitlab/auth/unique_ips_limiter_spec.rb @@ -0,0 +1,57 @@ +require 'spec_helper' + +describe Gitlab::Auth::UniqueIpsLimiter, :redis, lib: true do + include_context 'unique ips sign in limit' + let(:user) { create(:user) } + + describe '#count_unique_ips' do + context 'non unique IPs' do + it 'properly counts them' do + expect(described_class.update_and_return_ips_count(user.id, 'ip1')).to eq(1) + expect(described_class.update_and_return_ips_count(user.id, 'ip1')).to eq(1) + end + end + + context 'unique IPs' do + it 'properly counts them' do + expect(described_class.update_and_return_ips_count(user.id, 'ip2')).to eq(1) + expect(described_class.update_and_return_ips_count(user.id, 'ip3')).to eq(2) + end + end + + it 'resets count after specified time window' do + Timecop.freeze do + expect(described_class.update_and_return_ips_count(user.id, 'ip2')).to eq(1) + expect(described_class.update_and_return_ips_count(user.id, 'ip3')).to eq(2) + + Timecop.travel(Time.now.utc + described_class.config.unique_ips_limit_time_window) do + expect(described_class.update_and_return_ips_count(user.id, 'ip4')).to eq(1) + expect(described_class.update_and_return_ips_count(user.id, 'ip5')).to eq(2) + end + end + end + end + + describe '#limit_user!' do + include_examples 'user login operation with unique ip limit' do + def operation + described_class.limit_user! { user } + end + end + + context 'allow 2 unique ips' do + before { current_application_settings.update!(unique_ips_limit_per_user: 2) } + + it 'blocks user trying to login from third ip' do + change_ip('ip1') + expect(described_class.limit_user! { user }).to eq(user) + + change_ip('ip2') + expect(described_class.limit_user! { user }).to eq(user) + + change_ip('ip3') + expect { described_class.limit_user! { user } }.to raise_error(Gitlab::Auth::TooManyIps) + end + end + end +end diff --git a/spec/lib/gitlab/auth_spec.rb b/spec/lib/gitlab/auth_spec.rb index b234de4c772..daf8f5c1d6c 100644 --- a/spec/lib/gitlab/auth_spec.rb +++ b/spec/lib/gitlab/auth_spec.rb @@ -58,6 +58,14 @@ describe Gitlab::Auth, lib: true do expect(gl_auth.find_for_git_client(user.username, 'password', project: nil, ip: 'ip')).to eq(Gitlab::Auth::Result.new(user, nil, :gitlab_or_ldap, full_authentication_abilities)) end + include_examples 'user login operation with unique ip limit' do + let(:user) { create(:user, password: 'password') } + + def operation + expect(gl_auth.find_for_git_client(user.username, 'password', project: nil, ip: 'ip')).to eq(Gitlab::Auth::Result.new(user, nil, :gitlab_or_ldap, full_authentication_abilities)) + end + end + context 'while using LFS authenticate' do it 'recognizes user lfs tokens' do user = create(:user) @@ -196,6 +204,12 @@ describe Gitlab::Auth, lib: true do expect( gl_auth.find_with_user_password(username, password) ).not_to eql user end + include_examples 'user login operation with unique ip limit' do + def operation + expect(gl_auth.find_with_user_password(username, password)).to eq(user) + end + end + context "with ldap enabled" do before do allow(Gitlab::LDAP::Config).to receive(:enabled?).and_return(true) diff --git a/spec/lib/gitlab/request_context_spec.rb b/spec/lib/gitlab/request_context_spec.rb new file mode 100644 index 00000000000..a91c8655cdd --- /dev/null +++ b/spec/lib/gitlab/request_context_spec.rb @@ -0,0 +1,30 @@ +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 middleware run' do + it { is_expected.to be_nil } + end + end + end +end diff --git a/spec/requests/api/doorkeeper_access_spec.rb b/spec/requests/api/doorkeeper_access_spec.rb index bd9ecaf2685..2974875510a 100644 --- a/spec/requests/api/doorkeeper_access_spec.rb +++ b/spec/requests/api/doorkeeper_access_spec.rb @@ -1,17 +1,23 @@ require 'spec_helper' -describe API::API, api: true do +describe API::API, api: true do include ApiHelpers let!(:user) { create(:user) } let!(:application) { Doorkeeper::Application.create!(name: "MyApp", redirect_uri: "https://app.com", owner: user) } let!(:token) { Doorkeeper::AccessToken.create! application_id: application.id, resource_owner_id: user.id, scopes: "api" } - describe "when unauthenticated" do + describe "unauthenticated" do it "returns authentication success" do get api("/user"), access_token: token.token expect(response).to have_http_status(200) end + + include_examples 'user login request with unique ip limit' do + def request + get api('/user'), access_token: token.token + end + end end describe "when token invalid" do @@ -26,5 +32,11 @@ describe API::API, api: true do get api("/user", user) expect(response).to have_http_status(200) end + + include_examples 'user login request with unique ip limit' do + def request + get api('/user', user) + end + end end end diff --git a/spec/support/unique_ip_check_shared_examples.rb b/spec/support/unique_ip_check_shared_examples.rb new file mode 100644 index 00000000000..7cf5a65eeed --- /dev/null +++ b/spec/support/unique_ip_check_shared_examples.rb @@ -0,0 +1,62 @@ +shared_context 'unique ips sign in limit' do + include StubENV + before(:each) do + Gitlab::Redis.with(&:flushall) + end + + before do + stub_env('IN_MEMORY_APPLICATION_SETTINGS', 'false') + + current_application_settings.update!( + unique_ips_limit_enabled: true, + unique_ips_limit_time_window: 10000 + ) + end + + def change_ip(ip) + allow(Gitlab::RequestContext).to receive(:client_ip).and_return(ip) + end + + def request_from_ip(ip) + change_ip(ip) + request + response + end + + def operation_from_ip(ip) + change_ip(ip) + operation + end +end + +shared_examples 'user login operation with unique ip limit' do + include_context 'unique ips sign in limit' do + before { current_application_settings.update!(unique_ips_limit_per_user: 1) } + + it 'allows user authenticating from the same ip' do + expect { operation_from_ip('ip') }.not_to raise_error + expect { operation_from_ip('ip') }.not_to raise_error + end + + it 'blocks user authenticating from two distinct ips' do + expect { operation_from_ip('ip') }.not_to raise_error + expect { operation_from_ip('ip2') }.to raise_error(Gitlab::Auth::TooManyIps) + end + end +end + +shared_examples 'user login request with unique ip limit' do |success_status = 200| + include_context 'unique ips sign in limit' do + before { current_application_settings.update!(unique_ips_limit_per_user: 1) } + + it 'allows user authenticating from the same ip' do + expect(request_from_ip('ip')).to have_http_status(success_status) + expect(request_from_ip('ip')).to have_http_status(success_status) + end + + it 'blocks user authenticating from two distinct ips' do + expect(request_from_ip('ip')).to have_http_status(success_status) + expect(request_from_ip('ip2')).to have_http_status(403) + end + end +end