From 936f13af8f55dc3cb7f2c77a3bf098ecce6c63b6 Mon Sep 17 00:00:00 2001 From: Drew Blessing Date: Wed, 19 Oct 2022 15:40:04 -0500 Subject: [PATCH 1/2] Clean up and tie PBKDF2+SHA512 user passwords to FIPS Removes the feature flags associated with PBKDF2+SHA512 user passwords and ties it to FIPS. This also makes it clearer that the default is BCrypt and that is the way we intend to keep it. The prior implementation made it appear PBKDF2+SHA512 was the intended default. Changelog: changed --- .../concerns/encrypted_user_password.rb | 86 ++++++ app/models/user.rb | 49 +--- .../pbkdf2_password_encryption.yml | 8 - .../pbkdf2_password_encryption_write.yml | 8 - config/initializers/8_devise.rb | 8 - doc/security/password_storage.md | 14 +- .../concerns/encrypted_user_password_spec.rb | 144 ++++++++++ spec/models/user_spec.rb | 245 +----------------- 8 files changed, 243 insertions(+), 319 deletions(-) create mode 100644 app/models/concerns/encrypted_user_password.rb delete mode 100644 config/feature_flags/development/pbkdf2_password_encryption.yml delete mode 100644 config/feature_flags/development/pbkdf2_password_encryption_write.yml create mode 100644 spec/models/concerns/encrypted_user_password_spec.rb diff --git a/app/models/concerns/encrypted_user_password.rb b/app/models/concerns/encrypted_user_password.rb new file mode 100644 index 00000000000000..18a3ed28b2f71c --- /dev/null +++ b/app/models/concerns/encrypted_user_password.rb @@ -0,0 +1,86 @@ +# frozen_string_literal: true + +# Support for both BCrypt and PBKDF2+SHA512 user passwords +# Meant to be used exclusively with User model but extracted +# to a concern for isolation and clarity. +module EncryptedUserPassword + extend ActiveSupport::Concern + + BCRYPT_PREFIX = '$2a$' + PBKDF2_SHA512_PREFIX = '$pbkdf2-sha512$' + + BCRYPT_STRATEGY = :bcrypt + PBKDF2_SHA512_STRATEGY = :pbkdf2_sha512 + + # Use Devise DatabaseAuthenticatable#authenticatable_salt + # unless encrypted password is PBKDF2+SHA512. + def authenticatable_salt + return super unless pbkdf2_password? + + Devise::Pbkdf2Encryptable::Encryptors::Pbkdf2Sha512.split_digest(encrypted_password)[:salt] + end + + # Called by Devise during database authentication. + # Also migrates the user password to the configured + # encryption type (BCrypt or PBKDF2+SHA512), if needed. + def valid_password?(password) + return false unless password_matches?(password) + + migrate_password!(password) + end + + def password=(new_password) + @password = new_password # rubocop:disable Gitlab/ModuleWithInstanceVariables + return unless new_password.present? + + self.encrypted_password = if Gitlab::FIPS.enabled? + Devise::Pbkdf2Encryptable::Encryptors::Pbkdf2Sha512.digest( + new_password, + Devise::Pbkdf2Encryptable::Encryptors::Pbkdf2Sha512::STRETCHES, + Devise.friendly_token[0, 16]) + else + Devise::Encryptor.digest(self.class, new_password) + end + end + + private + + def password_strategy + case + when encrypted_password.starts_with?(BCRYPT_PREFIX) + BCRYPT_STRATEGY + when encrypted_password.starts_with?(PBKDF2_SHA512_PREFIX) + PBKDF2_SHA512_STRATEGY + else + :unknown + end + end + + def pbkdf2_password? + password_strategy == PBKDF2_SHA512_STRATEGY + end + + def bcrypt_password? + password_strategy == BCRYPT_STRATEGY + end + + def password_matches?(password) + if bcrypt_password? + Devise::Encryptor.compare(self.class, encrypted_password, password) + elsif pbkdf2_password? + Devise::Pbkdf2Encryptable::Encryptors::Pbkdf2Sha512.compare(encrypted_password, password) + end + end + + def migrate_password!(password) + return true if password_strategy == encryptor + + update_attribute(:password, password) + end + + def encryptor + return BCRYPT_STRATEGY unless Gitlab::FIPS.enabled? + + PBKDF2_SHA512_STRATEGY + end +end diff --git a/app/models/user.rb b/app/models/user.rb index 6d198fc755b58f..df234a7e0675e3 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -83,7 +83,10 @@ class User < ApplicationRecord serialize :otp_backup_codes, JSON # rubocop:disable Cop/ActiveRecordSerialize devise :lockable, :recoverable, :rememberable, :trackable, - :validatable, :omniauthable, :confirmable, :registerable, :pbkdf2_encryptable + :validatable, :omniauthable, :confirmable, :registerable + + # Must be included after `devise` + include EncryptedUserPassword include AdminChangedPasswordNotifier @@ -937,26 +940,14 @@ def recently_sent_password_reset? reset_password_sent_at.present? && reset_password_sent_at >= 1.minute.ago end - def authenticatable_salt - return encrypted_password[0, 29] unless Feature.enabled?(:pbkdf2_password_encryption) - return super if password_strategy == :pbkdf2_sha512 - - encrypted_password[0, 29] - end - # Overwrites valid_password? from Devise::Models::DatabaseAuthenticatable # In constant-time, check both that the password isn't on a denylist AND # that the password is the user's password def valid_password?(password) return false unless password_allowed?(password) return false if password_automatically_set? - return super if Feature.enabled?(:pbkdf2_password_encryption) - Devise::Encryptor.compare(self.class, encrypted_password, password) - rescue Devise::Pbkdf2Encryptable::Encryptors::InvalidHash - validate_and_migrate_bcrypt_password(password) - rescue ::BCrypt::Errors::InvalidHash - false + super end def generate_otp_backup_codes! @@ -975,27 +966,6 @@ def invalidate_otp_backup_code!(code) end end - # This method should be removed once the :pbkdf2_password_encryption feature flag is removed. - def password=(new_password) - if Feature.enabled?(:pbkdf2_password_encryption) && Feature.enabled?(:pbkdf2_password_encryption_write, self) - super - else - # Copied from Devise DatabaseAuthenticatable. - @password = new_password - self.encrypted_password = Devise::Encryptor.digest(self.class, new_password) if new_password.present? - end - end - - def password_strategy - super - rescue Devise::Pbkdf2Encryptable::Encryptors::InvalidHash - begin - return :bcrypt if BCrypt::Password.new(encrypted_password) - rescue BCrypt::Errors::InvalidHash - :unknown - end - end - # See https://gitlab.com/gitlab-org/security/gitlab/-/issues/638 DISALLOWED_PASSWORDS = %w[123qweQWE!@#000000000].freeze @@ -2440,15 +2410,6 @@ def ci_namespace_mirrors_for_group_members(level) Ci::NamespaceMirror.contains_traversal_ids(traversal_ids) end - - def validate_and_migrate_bcrypt_password(password) - return false unless Devise::Encryptor.compare(self.class, encrypted_password, password) - return true unless Feature.enabled?(:pbkdf2_password_encryption_write, self) - - update_attribute(:password, password) - rescue ::BCrypt::Errors::InvalidHash - false - end end User.prepend_mod_with('User') diff --git a/config/feature_flags/development/pbkdf2_password_encryption.yml b/config/feature_flags/development/pbkdf2_password_encryption.yml deleted file mode 100644 index 995173a6a38134..00000000000000 --- a/config/feature_flags/development/pbkdf2_password_encryption.yml +++ /dev/null @@ -1,8 +0,0 @@ ---- -name: pbkdf2_password_encryption -introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/91622 -rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/367147 -milestone: '15.2' -type: development -group: group::authentication and authorization -default_enabled: false diff --git a/config/feature_flags/development/pbkdf2_password_encryption_write.yml b/config/feature_flags/development/pbkdf2_password_encryption_write.yml deleted file mode 100644 index 29c7baedaf291d..00000000000000 --- a/config/feature_flags/development/pbkdf2_password_encryption_write.yml +++ /dev/null @@ -1,8 +0,0 @@ ---- -name: pbkdf2_password_encryption_write -introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/91622 -rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/367147 -milestone: '15.2' -type: development -group: group::authentication and authorization -default_enabled: false diff --git a/config/initializers/8_devise.rb b/config/initializers/8_devise.rb index 65314c4472fe6e..237231f544f8d7 100644 --- a/config/initializers/8_devise.rb +++ b/config/initializers/8_devise.rb @@ -178,14 +178,6 @@ # reset. Defaults to true, so a user is signed in automatically after a reset. config.sign_in_after_reset_password = false - # ==> Configuration for :encryptable - # Allow you to use another encryption algorithm besides bcrypt (default). You can use - # :sha1, :sha512 or encryptors from others authentication tools as :clearance_sha1, - # :authlogic_sha512 (then you should set stretches above to 20 for default behavior) - # and :restful_authentication_sha1 (then you should set stretches to 10, and copy - # REST_AUTH_SITE_KEY to pepper) - config.encryptor = :pbkdf2_sha512 - # Authentication through token does not store user in session and needs # to be supplied on each request. Useful if you are using the token as API token. config.skip_session_storage << :token_auth diff --git a/doc/security/password_storage.md b/doc/security/password_storage.md index 6b20f8619ae20d..0efa5b07ee06bd 100644 --- a/doc/security/password_storage.md +++ b/doc/security/password_storage.md @@ -11,7 +11,7 @@ GitLab administrators can configure how passwords and OAuth tokens are stored. ## Password storage -> PBKDF2 and SHA512 [introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/360658) in GitLab 15.2 [with flags](../administration/feature_flags.md) named `pbkdf2_password_encryption` and `pbkdf2_password_encryption_write`. Disabled by default. +> PBKDF2+SHA512 [introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/360658) in GitLab 15.2. Disabled by default. GitLab stores user passwords in a hashed format to prevent passwords from being stored as plain text. @@ -23,17 +23,7 @@ library to hash user passwords. Created password hashes have these attributes: - **BCrypt**: By default, the [`bcrypt`](https://en.wikipedia.org/wiki/Bcrypt) hashing function is used to generate the hash of the provided password. This cryptographic hashing function is strong and industry-standard. - - **PBKDF2 and SHA512**: Starting in GitLab 15.2, PBKDF2 and SHA512 are supported - behind the following feature flags (disabled by default): - - `pbkdf2_password_encryption` - Enables reading and comparison of PBKDF2 + SHA512 - hashed passwords and supports fallback for BCrypt hashed passwords. - - `pbkdf2_password_encryption_write` - Enables new passwords to be saved - using PBKDF2 and SHA512, and existing BCrypt passwords to be migrated when users sign in. - - FLAG: - On self-managed GitLab, by default this feature is not available. To make it available, - ask an administrator to [enable the feature flags](../administration/feature_flags.md) named `pbkdf2_password_encryption` and `pbkdf2_password_encryption_write`. - + - **PBKDF2+SHA512**: PBKDF2+SHA512 is supported when [FIPS mode](../development/fips_compliance.md) is enabled. - **Stretching**: Password hashes are [stretched](https://en.wikipedia.org/wiki/Key_stretching) to harden against brute-force attacks. By default, GitLab uses a stretching factor of 10 for BCrypt and 20,000 for PBKDF2 + SHA512. diff --git a/spec/models/concerns/encrypted_user_password_spec.rb b/spec/models/concerns/encrypted_user_password_spec.rb new file mode 100644 index 00000000000000..4b5f4a8446db45 --- /dev/null +++ b/spec/models/concerns/encrypted_user_password_spec.rb @@ -0,0 +1,144 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe User do + describe '#authenticatable_salt' do + let(:user) { build(:user, encrypted_password: encrypted_password) } + + subject(:authenticatable_salt) { user.authenticatable_salt } + + context 'when password is stored in BCrypt format' do + let(:encrypted_password) { '$2a$10$AvwDCyF/8HnlAv./UkAZx.vAlKRS89yNElP38FzdgOmVaSaiDL7xm' } + + it 'returns the first 30 characters of the encrypted_password' do + expect(authenticatable_salt).to eq(user.encrypted_password[0, 29]) + end + end + + context 'when password is stored in PBKDF2 format' do + let(:encrypted_password) { '$pbkdf2-sha512$20000$rKbYsScsDdk$iwWBewXmrkD2fFfaG1SDcMIvl9gvEo3fBWUAfiqyVceTlw/DYgKBByHzf45pF5Qn59R4R.NQHsFpvZB4qlsYmw' } # rubocop:disable Layout/LineLength + + it 'uses the decoded password salt' do + expect(authenticatable_salt).to eq('aca6d8b1272c0dd9') + end + + it 'does not use the first 30 characters of the encrypted_password' do + expect(authenticatable_salt).not_to eq(encrypted_password[0, 29]) + end + end + + context 'when the encrypted_password is an unknown type' do + let(:encrypted_password) { '$argon2i$v=19$m=512,t=4,p=2$eM+ZMyYkpDRGaI3xXmuNcQ$c5DeJg3eb5dskVt1mDdxfw' } + + it 'returns the first 30 characters of the encrypted_password' do + expect(authenticatable_salt).to eq(encrypted_password[0, 29]) + end + end + end + + describe '#valid_password?' do + subject(:validate_password) { user.valid_password?(password) } + + let(:user) { build(:user, encrypted_password: encrypted_password) } + let(:password) { described_class.random_password } + + shared_examples 'password validation fails when the password is encrypted using an unsupported method' do + let(:encrypted_password) { '$argon2i$v=19$m=512,t=4,p=2$eM+ZMyYkpDRGaI3xXmuNcQ$c5DeJg3eb5dskVt1mDdxfw' } + + it { is_expected.to eq(false) } + end + + context 'when the default encryption method is BCrypt' do + it_behaves_like 'password validation fails when the password is encrypted using an unsupported method' + + context 'when the user password PBKDF2+SHA512' do + let(:encrypted_password) do + Devise::Pbkdf2Encryptable::Encryptors::Pbkdf2Sha512.digest( + password, 20_000, Devise.friendly_token[0, 16]) + end + + it { is_expected.to eq(true) } + + it 're-encrypts the password as BCrypt' do + expect(user.encrypted_password).to start_with('$pbkdf2-sha512$') + + validate_password + + expect(user.encrypted_password).to start_with('$2a$') + end + end + end + + context 'when the default encryption method is PBKDF2+SHA512 and the user password is BCrypt', :fips_mode do + it_behaves_like 'password validation fails when the password is encrypted using an unsupported method' + + context 'when the user password BCrypt' do + let(:encrypted_password) { Devise::Encryptor.digest(described_class, password) } + + it { is_expected.to eq(true) } + + it 're-encrypts the password as PBKDF2+SHA512' do + expect(user.encrypted_password).to start_with('$2a$') + + validate_password + + expect(user.reload.encrypted_password).to start_with('$pbkdf2-sha512$') + end + end + end + end + + describe '#password=' do + let(:user) { build(:user) } + let(:password) { described_class.random_password } + + def compare_bcrypt_password(user, password) + Devise::Encryptor.compare(described_class, user.encrypted_password, password) + end + + def compare_pbkdf2_password(user, password) + Devise::Pbkdf2Encryptable::Encryptors::Pbkdf2Sha512.compare(user.encrypted_password, password) + end + + context 'when FIPS mode is enabled', :fips_mode do + it 'calls PBKDF2 digest and not the default Devise encryptor' do + expect(Devise::Pbkdf2Encryptable::Encryptors::Pbkdf2Sha512) + .to receive(:digest).at_least(:once).and_call_original + expect(Devise::Encryptor).not_to receive(:digest) + + user.password = password + end + + it 'saves the password in PBKDF2 format' do + user.password = password + user.save! + + expect(compare_pbkdf2_password(user, password)).to eq(true) + expect { compare_bcrypt_password(user, password) }.to raise_error(::BCrypt::Errors::InvalidHash) + end + end + + context 'when pbkdf2_password_encryption is disabled' do + before do + stub_feature_flags(pbkdf2_password_encryption: false) + end + + it 'calls default Devise encryptor and not the PBKDF2 encryptor' do + expect(Devise::Encryptor).to receive(:digest).at_least(:once).and_call_original + expect(Devise::Pbkdf2Encryptable::Encryptors::Pbkdf2Sha512).not_to receive(:digest) + + user.password = password + end + + it 'saves the password in BCrypt format' do + user.password = password + user.save! + + expect { compare_pbkdf2_password(user, password) } + .to raise_error Devise::Pbkdf2Encryptable::Encryptors::InvalidHash + expect(compare_bcrypt_password(user, password)).to eq(true) + end + end + end +end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 8ebf3d701654e1..84a23d86956f16 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -6157,172 +6157,28 @@ def access_levels(groups) end end - describe '#authenticatable_salt' do - let(:user) { create(:user) } - - subject(:authenticatable_salt) { user.authenticatable_salt } - - it 'uses password_salt' do - expect(authenticatable_salt).to eq(user.password_salt) - end - - context 'when the encrypted_password is an unknown type' do - let(:encrypted_password) { '$argon2i$v=19$m=512,t=4,p=2$eM+ZMyYkpDRGaI3xXmuNcQ$c5DeJg3eb5dskVt1mDdxfw' } - - before do - user.update_attribute(:encrypted_password, encrypted_password) - end - - it 'returns the first 30 characters of the encrypted_password' do - expect(authenticatable_salt).to eq(encrypted_password[0, 29]) - end - end - - context 'when pbkdf2_password_encryption is disabled' do - before do - stub_feature_flags(pbkdf2_password_encryption: false) - end - - it 'returns the first 30 characters of the encrypted_password' do - expect(authenticatable_salt).to eq(user.encrypted_password[0, 29]) - end - end - end - - def compare_pbkdf2_password(user, password) - Devise::Pbkdf2Encryptable::Encryptors::Pbkdf2Sha512.compare(user.encrypted_password, password) - end - describe '#valid_password?' do subject(:validate_password) { user.valid_password?(password) } - context 'user with password not in disallowed list' do - let(:user) { create(:user) } - let(:password) { user.password } - - it { is_expected.to be_truthy } - - context 'using a wrong password' do - let(:password) { 'WRONG PASSWORD' } - - it { is_expected.to be_falsey } - end - - context 'when pbkdf2_sha512_encryption is disabled and the user password is pbkdf2+sha512' do - it 'does not validate correctly' do - user # Create the user while the feature is enabled - stub_feature_flags(pbkdf2_password_encryption: false) - - expect(validate_password).to be_falsey - end - end - end - context 'user with disallowed password' do let(:user) { create(:user, :disallowed_password) } let(:password) { user.password } - it { is_expected.to be_falsey } - - context 'using a wrong password' do - let(:password) { 'WRONG PASSWORD' } - - it { is_expected.to be_falsey } - end - end - - context 'user with a bcrypt password hash' do - # Manually set a 'known' encrypted password - let(:password) { User.random_password } - let(:encrypted_password) { Devise::Encryptor.digest(User, password) } - let(:user) { create(:user, encrypted_password: encrypted_password) } - - shared_examples 'not re-encrypting with PBKDF2' do - it 'does not re-encrypt with PBKDF2' do - validate_password - - expect(user.reload.encrypted_password).to eq(encrypted_password) - end - end - - context 'using the wrong password' do - # password 'WRONG PASSWORD' will not match the bcrypt hash - let(:password) { 'WRONG PASSWORD' } - let(:encrypted_password) { Devise::Encryptor.digest(User, User.random_password) } - - it { is_expected.to be_falsey } - - it_behaves_like 'not re-encrypting with PBKDF2' - - context 'when pbkdf2_password_encryption is disabled' do - before do - stub_feature_flags(pbkdf2_password_encryption: false) - end - - it { is_expected.to be_falsey } - - it_behaves_like 'not re-encrypting with PBKDF2' - end - end - - context 'using the correct password' do - it { is_expected.to be_truthy } - - it 'validates the password and re-encrypts with PBKDF2' do - validate_password - - current_encrypted_password = user.reload.encrypted_password - - expect(compare_pbkdf2_password(user, password)).to eq(true) - expect { ::BCrypt::Password.new(current_encrypted_password) } - .to raise_error(::BCrypt::Errors::InvalidHash) - end - - context 'when pbkdf2_password_encryption is disabled' do - before do - stub_feature_flags(pbkdf2_password_encryption: false) - end - - it { is_expected.to be_truthy } - - it_behaves_like 'not re-encrypting with PBKDF2' - end - - context 'when pbkdf2_password_encryption_write is disabled' do - before do - stub_feature_flags(pbkdf2_password_encryption_write: false) - end - - it { is_expected.to be_truthy } - - it_behaves_like 'not re-encrypting with PBKDF2' - end - end + it { is_expected.to eq(false) } end - context 'user with password hash that is neither PBKDF2 nor BCrypt' do - # Manually calculated User.random_password - let(:password) { "gg_w215TmVXGWSt7RJKXwYTVz886f6SDM3zvzztaJf2mX9ttUE8gRkNJSbWyWRLqxz4LFzxBekPe75ydDcGauE9wqg-acKMRT-WpSYjTm1Rdx-tnssE7CQByJcnxwWNH" } - # Created with https://argon2.online/ using 'aaaaaaaa' as the salt - let(:encrypted_password) { "$argon2i$v=19$m=512,t=4,p=2$YWFhYWFhYWE$PvJscKO5XRlevcgRReUg6w" } - let(:user) { create(:user, encrypted_password: encrypted_password) } - - it { is_expected.to be_falsey } - - context 'when pbkdf2_password_encryption is disabled' do - before do - stub_feature_flags(pbkdf2_password_encryption: false) - end + context 'using a wrong password' do + let(:user) { create(:user) } + let(:password) { 'WRONG PASSWORD' } - it { is_expected.to be_falsey } - end + it { is_expected.to eq(false) } end context 'user with autogenerated_password' do let(:user) { build_stubbed(:user, password_automatically_set: true) } let(:password) { user.password } - it { is_expected.to be_falsey } + it { is_expected.to eq(false) } end end @@ -6377,95 +6233,6 @@ def compare_pbkdf2_password(user, password) end end - # These entire test section can be removed once the :pbkdf2_password_encryption feature flag is removed. - describe '#password=' do - let(:user) { create(:user) } - let(:password) { User.random_password } - - def compare_bcrypt_password(user, password) - Devise::Encryptor.compare(User, user.encrypted_password, password) - end - - context 'when pbkdf2_password_encryption is enabled' do - it 'calls PBKDF2 digest and not the default Devise encryptor' do - expect(Devise::Pbkdf2Encryptable::Encryptors::Pbkdf2Sha512).to receive(:digest).at_least(:once).and_call_original - expect(Devise::Encryptor).not_to receive(:digest) - - user.password = password - end - - it 'saves the password in PBKDF2 format' do - user.password = password - user.save! - - expect(compare_pbkdf2_password(user, password)).to eq(true) - expect { compare_bcrypt_password(user, password) }.to raise_error(::BCrypt::Errors::InvalidHash) - end - - context 'when pbkdf2_password_encryption_write is disabled' do - before do - stub_feature_flags(pbkdf2_password_encryption_write: false) - end - - it 'calls default Devise encryptor and not the PBKDF2 encryptor' do - expect(Devise::Encryptor).to receive(:digest).at_least(:once).and_call_original - expect(Devise::Pbkdf2Encryptable::Encryptors::Pbkdf2Sha512).not_to receive(:digest) - - user.password = password - end - end - end - - context 'when pbkdf2_password_encryption is disabled' do - before do - stub_feature_flags(pbkdf2_password_encryption: false) - end - - it 'calls default Devise encryptor and not the PBKDF2 encryptor' do - expect(Devise::Encryptor).to receive(:digest).at_least(:once).and_call_original - expect(Devise::Pbkdf2Encryptable::Encryptors::Pbkdf2Sha512).not_to receive(:digest) - - user.password = password - end - - it 'saves the password in BCrypt format' do - user.password = password - user.save! - - expect { compare_pbkdf2_password(user, password) }.to raise_error Devise::Pbkdf2Encryptable::Encryptors::InvalidHash - expect(compare_bcrypt_password(user, password)).to eq(true) - end - end - end - - describe '#password_strategy' do - let(:user) { create(:user, encrypted_password: encrypted_password) } - - context 'with a PBKDF2+SHA512 encrypted password' do - let(:encrypted_password) { '$pbkdf2-sha512$20000$boHGAw0hEyI$DBA67J7zNZebyzLtLk2X9wRDbmj1LNKVGnZLYyz6PGrIDGIl45fl/BPH0y1TPZnV90A20i.fD9C3G9Bp8jzzOA' } - - it 'extracts the correct strategy', :aggregate_failures do - expect(user.password_strategy).to eq(:pbkdf2_sha512) - end - end - - context 'with a BCrypt encrypted password' do - let(:encrypted_password) { '$2a$10$xLTxCKOa75IU4RQGqqOrTuZOgZdJEzfSzjG6ZSEi/C31TB/yLZYpi' } - - it 'extracts the correct strategy', :aggregate_failures do - expect(user.password_strategy).to eq(:bcrypt) - end - end - - context 'with an unknown encrypted password' do - let(:encrypted_password) { '$pbkdf2-sha256$6400$.6UI/S.nXIk8jcbdHx3Fhg$98jZicV16ODfEsEZeYPGHU3kbrUrvUEXOPimVSQDD44' } - - it 'returns unknown strategy' do - expect(user.password_strategy).to eq(:unknown) - end - end - end - describe '#password_expired?' do let(:user) { build(:user, password_expires_at: password_expires_at) } -- GitLab From 76ad643a3d4b4f12bcdd3ebb4317cdc1237af4eb Mon Sep 17 00:00:00 2001 From: Evan Read Date: Tue, 25 Oct 2022 14:10:47 +0000 Subject: [PATCH 2/2] Apply 2 suggestion(s) to 1 file(s) --- app/models/concerns/encrypted_user_password.rb | 12 ++++-------- doc/security/password_storage.md | 7 +++++-- 2 files changed, 9 insertions(+), 10 deletions(-) diff --git a/app/models/concerns/encrypted_user_password.rb b/app/models/concerns/encrypted_user_password.rb index 18a3ed28b2f71c..97e6592f442641 100644 --- a/app/models/concerns/encrypted_user_password.rb +++ b/app/models/concerns/encrypted_user_password.rb @@ -46,14 +46,10 @@ def password=(new_password) private def password_strategy - case - when encrypted_password.starts_with?(BCRYPT_PREFIX) - BCRYPT_STRATEGY - when encrypted_password.starts_with?(PBKDF2_SHA512_PREFIX) - PBKDF2_SHA512_STRATEGY - else - :unknown - end + return BCRYPT_STRATEGY if encrypted_password.starts_with?(BCRYPT_PREFIX) + return PBKDF2_SHA512_STRATEGY if encrypted_password.starts_with?(PBKDF2_SHA512_PREFIX) + + :unknown end def pbkdf2_password? diff --git a/doc/security/password_storage.md b/doc/security/password_storage.md index 0efa5b07ee06bd..279f7a9359eb9e 100644 --- a/doc/security/password_storage.md +++ b/doc/security/password_storage.md @@ -11,7 +11,8 @@ GitLab administrators can configure how passwords and OAuth tokens are stored. ## Password storage -> PBKDF2+SHA512 [introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/360658) in GitLab 15.2. Disabled by default. +> - PBKDF2+SHA512 [introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/360658) in GitLab 15.2 [with flags](../administration/feature_flags.md) named `pbkdf2_password_encryption` and `pbkdf2_password_encryption_write`. Disabled by default. +> - Feature flags [removed](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/101691) in GitLab 15.6 and PBKDF2+SHA512 was made available to all GitLab instances running in [FIPS mode](../development/fips_compliance.md). GitLab stores user passwords in a hashed format to prevent passwords from being stored as plain text. @@ -23,7 +24,9 @@ library to hash user passwords. Created password hashes have these attributes: - **BCrypt**: By default, the [`bcrypt`](https://en.wikipedia.org/wiki/Bcrypt) hashing function is used to generate the hash of the provided password. This cryptographic hashing function is strong and industry-standard. - - **PBKDF2+SHA512**: PBKDF2+SHA512 is supported when [FIPS mode](../development/fips_compliance.md) is enabled. + - **PBKDF2+SHA512**: PBKDF2+SHA512 is supported: + - In GitLab 15.2 to GitLab 15.5 when `pbkdf2_password_encryption` and `pbkdf2_password_encryption_write` [feature flags](../administration/feature_flags.md) are enabled. + - In GitLab 15.6 and later when [FIPS mode](../development/fips_compliance.md) is enabled (feature flags are not required). - **Stretching**: Password hashes are [stretched](https://en.wikipedia.org/wiki/Key_stretching) to harden against brute-force attacks. By default, GitLab uses a stretching factor of 10 for BCrypt and 20,000 for PBKDF2 + SHA512. -- GitLab