Merge branch 'boconnor/update-bcrypt-work-factor' into 'master'

Update bcrypt work factor to 13

See merge request https://gitlab.com/gitlab-org/gitlab/-/merge_requests/177154



Merged-by: Drew Blessing <drew@gitlab.com>
Approved-by: Drew Blessing <drew@gitlab.com>
Reviewed-by: Drew Blessing <drew@gitlab.com>
Reviewed-by: Imre Farkas <ifarkas@gitlab.com>
Reviewed-by: Ash McKenzie <amckenzie@gitlab.com>
Co-authored-by: Brendan O'Connor <boconnor@gitlab.com>

app/models/concerns/encrypted_user_password.rb

@@ -12,6 +12,16 @@ module EncryptedUserPassword
   BCRYPT_STRATEGY = :bcrypt
   PBKDF2_SHA512_STRATEGY = :pbkdf2_sha512
 
+  class_methods do
+    def stretches
+      prior_stretches = Rails.env.test? ? 1 : 10
+
+      return prior_stretches unless Feature.enabled?(:increase_password_storage_stretches) # rubocop:disable Gitlab/FeatureFlagWithoutActor -- required to enable FFing a Class method, which is required to FF the Stretches config
+
+      Rails.env.test? ? 5 : 13
+    end
+  end
+
   # Use Devise DatabaseAuthenticatable#authenticatable_salt
   # unless encrypted password is PBKDF2+SHA512.
   def authenticatable_salt
@@ -69,11 +79,27 @@ def password_matches?(password)
   end
 
   def migrate_password!(password)
-    return true if password_strategy == encryptor
+    # A note on ordering here:
+    # Other code expects to use this function to switch between pbkdf2 and bcrypt.
+    # Hence, if password strategy != encryptor, we need to fail immediately and migrate.
+    # Reversing this ordering will break tests in spec/models/concerns/encrypted_user_password_spec.rb.
+
+    if password_strategy == encryptor
+      return true unless Feature.enabled?(:increase_password_storage_stretches) # rubocop:disable Gitlab/FeatureFlagWithoutActor -- required to enable FFing a Class method, which is required to FF the Stretches config
+      return true if (BCRYPT_STRATEGY == password_strategy) && bcrypt_password_matches_current_stretches?
+      # We do not attempt to upgrade stretches on PBKDF2-stored passwords.
+      return true if PBKDF2_SHA512_STRATEGY == password_strategy
+    end
 
     update_attribute(:password, password)
   end
 
+  def bcrypt_password_matches_current_stretches?
+    return false unless bcrypt_password?
+
+    ::BCrypt::Password.new(encrypted_password).cost == self.class.stretches
+  end
+
   def encryptor
     return BCRYPT_STRATEGY unless Gitlab::FIPS.enabled?
 

config/feature_flags/gitlab_com_derisk/increase_password_storage_stretches.yml

+---
+name: increase_password_storage_stretches
+feature_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/222481
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/177154
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/511397
+milestone: '17.9'
+group: group::authentication
+type: gitlab_com_derisk
+default_enabled: false

config/initializers/8_devise.rb

@@ -80,6 +80,13 @@
   # Limiting the stretches to just one in testing will increase the performance of
   # your test suite dramatically. However, it is STRONGLY RECOMMENDED to not use
   # a value less than 10 in other environments.
+
+  # The bcrypt gem does not allow stretches to be set less than 4 (it will ignore it).
+  # To allow password WF upgrade testing (spec/models/concerns/encrypted_user_password_spec.rb),
+  # changing the test-side configuration to 5 to give the test something to do,
+  # along with changing the production value to 13 for https://gitlab.com/gitlab-org/gitlab/-/issues/222481.
+  # config.stretches = Rails.env.test? ? 5 : 13
+  # NOTE: This is being overridden in the `encrypted_user_password.rb` concern, behind an FF
   config.stretches = Rails.env.test? ? 1 : 10
 
   # Set up a pepper to generate the encrypted password.

spec/models/concerns/encrypted_user_password_spec.rb

@@ -2,7 +2,7 @@
 
 require 'spec_helper'
 
-RSpec.describe User do
+RSpec.describe User, feature_category: :system_access do
   describe '#authenticatable_salt' do
     let(:user) { build(:user, encrypted_password: encrypted_password) }
 
@@ -53,6 +53,35 @@
     end
 
     context 'when the default encryption method is BCrypt' do
+      context 'when the user password is hashed with work factor 4' do
+        let(:encrypted_password) { "$2a$04$ThzqXSFnlW3uH86uQ79puOU7vARSFuuNzb1nUGfsBeYtCLkdymAQW" }
+        let(:increase_password_storage_stretches) { nil }
+
+        before do
+          stub_feature_flags(increase_password_storage_stretches: increase_password_storage_stretches)
+        end
+
+        context 'when feature flag is set to true' do
+          let(:increase_password_storage_stretches) { true }
+
+          it 'upgrades stretches' do
+            expect(user.encrypted_password).to start_with('$2a$04$')
+            user.valid_password?('security')
+            expect(user.encrypted_password).to start_with('$2a$05$')
+          end
+        end
+
+        context 'when feature flag is set to false' do
+          let(:increase_password_storage_stretches) { false }
+
+          it 'does not upgrade stretches' do
+            expect(user.encrypted_password).to start_with('$2a$04$')
+            user.valid_password?('security')
+            expect(user.encrypted_password).to start_with('$2a$04$')
+          end
+        end
+      end
+
       it_behaves_like 'password validation fails when the password is encrypted using an unsupported method'
 
       context 'when the user password PBKDF2+SHA512' do