summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorGitLab Release Tools Bot <delivery-team+release-tools@gitlab.com>2020-09-02 21:36:11 +0000
committerGitLab Release Tools Bot <delivery-team+release-tools@gitlab.com>2020-09-02 21:36:11 +0000
commit517f254952ababb661160d3afd659902d18e29cd (patch)
tree99d3e75511ff0e067eb6bbb5082dd56caf34c1e1
parent90432d32acd69cf91e647fc508045659cae26b1a (diff)
parentaebc07f48e86bb27f3fdf7992f516e0ba73abecf (diff)
downloadgitlab-ce-517f254952ababb661160d3afd659902d18e29cd.tar.gz
Merge remote-tracking branch 'dev/13-3-stable' into 13-3-stable
-rw-r--r--CHANGELOG.md7
-rw-r--r--GITALY_SERVER_VERSION2
-rw-r--r--VERSION2
-rw-r--r--config/initializers/doorkeeper.rb2
-rw-r--r--db/migrate/20200728182311_add_o_auth_paths_to_protected_paths.rb62
-rw-r--r--db/schema_migrations/202007281823111
-rw-r--r--db/structure.sql2
-rw-r--r--lib/gitlab/auth.rb23
-rw-r--r--spec/lib/gitlab/auth_spec.rb57
-rw-r--r--spec/migrations/20200728182311_add_o_auth_paths_to_protected_paths_spec.rb52
10 files changed, 204 insertions, 6 deletions
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 89c328b93d3..652f9f8a2a6 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -2,6 +2,13 @@
documentation](doc/development/changelog.md) for instructions on adding your own
entry.
+## 13.3.4 (2020-09-02)
+
+### Security (1 change)
+
+- Protect OAuth endpoints from brute force/password stuffing.
+
+
## 13.3.3 (2020-09-02)
### Security (23 changes, 1 of them is from the community)
diff --git a/GITALY_SERVER_VERSION b/GITALY_SERVER_VERSION
index 35f1a3e990f..51d9561af40 100644
--- a/GITALY_SERVER_VERSION
+++ b/GITALY_SERVER_VERSION
@@ -1 +1 @@
-13.3.3 \ No newline at end of file
+13.3.4
diff --git a/VERSION b/VERSION
index 4be057655b4..51d9561af40 100644
--- a/VERSION
+++ b/VERSION
@@ -1 +1 @@
-13.3.3
+13.3.4
diff --git a/config/initializers/doorkeeper.rb b/config/initializers/doorkeeper.rb
index 76e29fb6c02..ad0b0c2008f 100644
--- a/config/initializers/doorkeeper.rb
+++ b/config/initializers/doorkeeper.rb
@@ -17,7 +17,7 @@ Doorkeeper.configure do
end
resource_owner_from_credentials do |routes|
- user = Gitlab::Auth.find_with_user_password(params[:username], params[:password])
+ user = Gitlab::Auth.find_with_user_password(params[:username], params[:password], increment_failed_attempts: true)
user unless user.try(:two_factor_enabled?)
end
diff --git a/db/migrate/20200728182311_add_o_auth_paths_to_protected_paths.rb b/db/migrate/20200728182311_add_o_auth_paths_to_protected_paths.rb
new file mode 100644
index 00000000000..7a5af0135fa
--- /dev/null
+++ b/db/migrate/20200728182311_add_o_auth_paths_to_protected_paths.rb
@@ -0,0 +1,62 @@
+# frozen_string_literal: true
+
+class AddOAuthPathsToProtectedPaths < ActiveRecord::Migration[6.0]
+ include Gitlab::Database::MigrationHelpers
+
+ DOWNTIME = false
+
+ ADD_PROTECTED_PATHS = [
+ '/oauth/authorize',
+ '/oauth/token'
+ ].freeze
+
+ EXISTING_DEFAULT_PROTECTED_PATHS = [
+ '/users/password',
+ '/users/sign_in',
+ '/api/v3/session.json',
+ '/api/v3/session',
+ '/api/v4/session.json',
+ '/api/v4/session',
+ '/users',
+ '/users/confirmation',
+ '/unsubscribes/',
+ '/import/github/personal_access_token',
+ '/admin/session'
+ ].freeze
+
+ NEW_DEFAULT_PROTECTED_PATHS = (EXISTING_DEFAULT_PROTECTED_PATHS + ADD_PROTECTED_PATHS).freeze
+
+ class ApplicationSetting < ActiveRecord::Base
+ self.table_name = 'application_settings'
+ end
+
+ def up
+ change_column_default :application_settings, :protected_paths, NEW_DEFAULT_PROTECTED_PATHS
+
+ ApplicationSetting.reset_column_information
+
+ ApplicationSetting.where.not(protected_paths: nil).each do |application_setting|
+ missing_paths = ADD_PROTECTED_PATHS - application_setting.protected_paths
+
+ next if missing_paths.empty?
+
+ updated_protected_paths = application_setting.protected_paths + missing_paths
+ application_setting.update!(protected_paths: updated_protected_paths)
+ end
+ end
+
+ def down
+ change_column_default :application_settings, :protected_paths, EXISTING_DEFAULT_PROTECTED_PATHS
+
+ ApplicationSetting.reset_column_information
+
+ ApplicationSetting.where.not(protected_paths: nil).each do |application_setting|
+ paths_to_remove = application_setting.protected_paths - EXISTING_DEFAULT_PROTECTED_PATHS
+
+ next if paths_to_remove.empty?
+
+ updated_protected_paths = application_setting.protected_paths - paths_to_remove
+ application_setting.update!(protected_paths: updated_protected_paths)
+ end
+ end
+end
diff --git a/db/schema_migrations/20200728182311 b/db/schema_migrations/20200728182311
new file mode 100644
index 00000000000..6bb5a869513
--- /dev/null
+++ b/db/schema_migrations/20200728182311
@@ -0,0 +1 @@
+2aab4599404312ddcc5bc9af11b0a21dfd6aa8aa10d4b4b5086a93ce1ffe77b6 \ No newline at end of file
diff --git a/db/structure.sql b/db/structure.sql
index 3e3014da914..950f7930a38 100644
--- a/db/structure.sql
+++ b/db/structure.sql
@@ -9174,7 +9174,7 @@ CREATE TABLE public.application_settings (
throttle_protected_paths_enabled boolean DEFAULT false NOT NULL,
throttle_protected_paths_requests_per_period integer DEFAULT 10 NOT NULL,
throttle_protected_paths_period_in_seconds integer DEFAULT 60 NOT NULL,
- protected_paths character varying(255)[] DEFAULT '{/users/password,/users/sign_in,/api/v3/session.json,/api/v3/session,/api/v4/session.json,/api/v4/session,/users,/users/confirmation,/unsubscribes/,/import/github/personal_access_token,/admin/session}'::character varying[],
+ protected_paths character varying(255)[] DEFAULT '{/users/password,/users/sign_in,/api/v3/session.json,/api/v3/session,/api/v4/session.json,/api/v4/session,/users,/users/confirmation,/unsubscribes/,/import/github/personal_access_token,/admin/session,/oauth/authorize,/oauth/token}'::character varying[],
throttle_incident_management_notification_enabled boolean DEFAULT false NOT NULL,
throttle_incident_management_notification_period_in_seconds integer DEFAULT 3600,
throttle_incident_management_notification_per_period integer DEFAULT 3600,
diff --git a/lib/gitlab/auth.rb b/lib/gitlab/auth.rb
index 0c6ed6924bf..ece4946383d 100644
--- a/lib/gitlab/auth.rb
+++ b/lib/gitlab/auth.rb
@@ -65,7 +65,15 @@ module Gitlab
raise Gitlab::Auth::MissingPersonalAccessTokenError
end
- def find_with_user_password(login, password)
+ # Find and return a user if the provided password is valid for various
+ # authenticators (OAuth, LDAP, Local Database).
+ #
+ # Specify `increment_failed_attempts: true` to increment Devise `failed_attempts`.
+ # CAUTION: Avoid incrementing failed attempts when authentication falls through
+ # different mechanisms, as in `.find_for_git_client`. This may lead to
+ # unwanted access locks when the value provided for `password` was actually
+ # a PAT, deploy token, etc.
+ def find_with_user_password(login, password, increment_failed_attempts: false)
# Avoid resource intensive checks if login credentials are not provided
return unless login.present? && password.present?
@@ -96,10 +104,14 @@ module Gitlab
authenticators.compact!
# return found user that was authenticated first for given login credentials
- authenticators.find do |auth|
+ authenticated_user = authenticators.find do |auth|
authenticated_user = auth.login(login, password)
break authenticated_user if authenticated_user
end
+
+ user_auth_attempt!(user, success: !!authenticated_user) if increment_failed_attempts
+
+ authenticated_user
end
end
@@ -357,6 +369,13 @@ module Gitlab
def find_build_by_token(token)
::Ci::Build.running.find_by_token(token)
end
+
+ def user_auth_attempt!(user, success:)
+ return unless user && Gitlab::Database.read_write?
+ return user.unlock_access! if success
+
+ user.increment_failed_attempts!
+ end
end
end
end
diff --git a/spec/lib/gitlab/auth_spec.rb b/spec/lib/gitlab/auth_spec.rb
index 3bd35fb83fd..b6a8ac31074 100644
--- a/spec/lib/gitlab/auth_spec.rb
+++ b/spec/lib/gitlab/auth_spec.rb
@@ -691,12 +691,69 @@ RSpec.describe Gitlab::Auth, :use_clean_rails_memory_store_caching do
expect( gl_auth.find_with_user_password(username, password) ).not_to eql user
end
+ it 'does not find user in locked state' do
+ user.lock_access!
+
+ expect(gl_auth.find_with_user_password(username, password)).not_to eql user
+ end
+
it "does not find user in ldap_blocked state" do
user.ldap_block
expect( gl_auth.find_with_user_password(username, password) ).not_to eql user
end
+ context 'with increment_failed_attempts' do
+ wrong_password = 'incorrect_password'
+
+ it 'increments failed_attempts when true and password is incorrect' do
+ expect do
+ gl_auth.find_with_user_password(username, wrong_password, increment_failed_attempts: true)
+ user.reload
+ end.to change(user, :failed_attempts).from(0).to(1)
+ end
+
+ it 'resets failed_attempts when true and password is correct' do
+ user.failed_attempts = 2
+ user.save
+
+ expect do
+ gl_auth.find_with_user_password(username, password, increment_failed_attempts: true)
+ user.reload
+ end.to change(user, :failed_attempts).from(2).to(0)
+ end
+
+ it 'does not increment failed_attempts by default' do
+ expect do
+ gl_auth.find_with_user_password(username, wrong_password)
+ user.reload
+ end.not_to change(user, :failed_attempts)
+ end
+
+ context 'when the database is read only' do
+ before do
+ allow(Gitlab::Database).to receive(:read_only?).and_return(true)
+ end
+
+ it 'does not increment failed_attempts when true and password is incorrect' do
+ expect do
+ gl_auth.find_with_user_password(username, wrong_password, increment_failed_attempts: true)
+ user.reload
+ end.not_to change(user, :failed_attempts)
+ end
+
+ it 'does not reset failed_attempts when true and password is correct' do
+ user.failed_attempts = 2
+ user.save
+
+ expect do
+ gl_auth.find_with_user_password(username, password, increment_failed_attempts: true)
+ user.reload
+ end.not_to change(user, :failed_attempts)
+ end
+ end
+ end
+
context "with ldap enabled" do
before do
allow(Gitlab::Auth::Ldap::Config).to receive(:enabled?).and_return(true)
diff --git a/spec/migrations/20200728182311_add_o_auth_paths_to_protected_paths_spec.rb b/spec/migrations/20200728182311_add_o_auth_paths_to_protected_paths_spec.rb
new file mode 100644
index 00000000000..e12519e15b8
--- /dev/null
+++ b/spec/migrations/20200728182311_add_o_auth_paths_to_protected_paths_spec.rb
@@ -0,0 +1,52 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+require Rails.root.join('db', 'migrate', '20200728182311_add_o_auth_paths_to_protected_paths.rb')
+
+RSpec.describe AddOAuthPathsToProtectedPaths do
+ subject(:migration) { described_class.new }
+
+ let(:application_settings) { table(:application_settings) }
+ let(:new_paths) do
+ [
+ '/oauth/authorize',
+ '/oauth/token'
+ ]
+ end
+
+ it 'appends new OAuth paths' do
+ application_settings.create!
+
+ protected_paths_before = application_settings.first.protected_paths
+ protected_paths_after = protected_paths_before + new_paths
+
+ expect { migrate! }.to change { application_settings.first.protected_paths }.from(protected_paths_before).to(protected_paths_after)
+ end
+
+ it 'new default includes new paths' do
+ settings_before = application_settings.create!
+
+ expect(settings_before.protected_paths).not_to include(*new_paths)
+
+ migrate!
+
+ application_settings.reset_column_information
+ settings_after = application_settings.create!
+
+ expect(settings_after.protected_paths).to include(*new_paths)
+ end
+
+ it 'does not change the value when the new paths are already included' do
+ application_settings.create!(protected_paths: %w(/users/sign_in /users/password) + new_paths)
+
+ expect { migrate! }.not_to change { application_settings.first.protected_paths }
+ end
+
+ it 'adds one value when the other is already present' do
+ application_settings.create!(protected_paths: %W(/users/sign_in /users/password #{new_paths.first}))
+
+ migrate!
+
+ expect(application_settings.first.protected_paths).to include(new_paths.second)
+ end
+end