Merge branch '27520-option-to-prevent-signing-in-from-multiple-ips' into 'master'

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,
 

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

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,

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

changelogs/unreleased/27520-option-to-prevent-signing-in-from-multiple-ips.yml

+---
+title: Option to prevent signing in from multiple ips
+merge_request: 8998
+author:

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

config/initializers/request_context.rb

+Rails.application.configure do |config|
+  config.middleware.insert_after RequestStore::Middleware, Gitlab::RequestContext
+end

config/initializers/warden.rb

+Rails.application.configure do |config|
+  Warden::Manager.after_set_user do |user, auth, opts|
+    Gitlab::Auth::UniqueIpsLimiter.limit_user!(user)
+  end
+end

db/migrate/20170210131347_add_unique_ips_limit_to_application_settings.rb

+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

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|

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

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!

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
 

lib/gitlab/auth/too_many_ips.rb

+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

lib/gitlab/auth/unique_ips_limiter.rb

+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

lib/gitlab/request_context.rb

+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

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

spec/lib/gitlab/auth/unique_ips_limiter_spec.rb

+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

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)

spec/lib/gitlab/request_context_spec.rb

+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

spec/requests/api/doorkeeper_access_spec.rb

 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

spec/support/unique_ip_check_shared_examples.rb

+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