diff --git a/app/jobs/backup_code_backfiller_job.rb b/app/jobs/backup_code_backfiller_job.rb deleted file mode 100644 index a96db03a867..00000000000 --- a/app/jobs/backup_code_backfiller_job.rb +++ /dev/null @@ -1,49 +0,0 @@ -class BackupCodeBackfillerJob < ApplicationJob - queue_as :long_running - - # Helper to be run manually in a Rails console once - def self.enqueue_all(batch_size: 10_000) - Range.new(1, User.last.id). - step(batch_size). - each do |start| - perform_later(start_id: start, count: batch_size) - end - end - - # Operates on a batch by user, so that we can give a batch - # the same salt to make forward lookups more efficient - def perform(start_id:, count:) - User. - includes(:backup_code_configurations). - where('id >= ?', start_id). - order(:id). - limit(count). - each do |user| - perform_batch(user.backup_code_configurations) - rescue => e - if Rails.env.production? - Rails.logger.warn("error converting backup codes for user_id=#{user.id} #{e}") - else - raise e - end - end - end - - def perform_batch(backup_code_configurations) - configs_with_legacy_code = backup_code_configurations.select { |b| b.encrypted_code.present? } - - return if configs_with_legacy_code.empty? - - # @see BackupCodeGenerator#save - salt = SecureRandom.hex(32) - - configs_with_legacy_code.each do |backup_code_configuration| - backup_code_configuration.update( - skip_legacy_encryption: true, - code_cost: IdentityConfig.store.backup_code_cost, - code_salt: salt, - code: backup_code_configuration.code, - ) - end - end -end diff --git a/app/models/backup_code_configuration.rb b/app/models/backup_code_configuration.rb index b81bc80ce20..ece04519ee7 100644 --- a/app/models/backup_code_configuration.rb +++ b/app/models/backup_code_configuration.rb @@ -1,6 +1,8 @@ class BackupCodeConfiguration < ApplicationRecord NUM_WORDS = 3 + self.ignored_columns = %w[encrypted_code code_fingerprint] + include EncryptableAttribute encrypted_attribute_without_setter(name: :code) @@ -9,9 +11,6 @@ class BackupCodeConfiguration < ApplicationRecord belongs_to :user - attr_accessor :skip_legacy_encryption - alias_method :skip_legacy_encryption?, :skip_legacy_encryption - def self.unused where(used_at: nil) end @@ -51,11 +50,7 @@ def find_with_code(code:, user_id:) scrypt_password_digest(password: code, salt: salt, cost: cost) end - where( - code_fingerprint: create_fingerprint(code), - ).or( - where(salted_code_fingerprint: salted_fingerprints), - ).find_by(user_id: user_id) + where(salted_code_fingerprint: salted_fingerprints).find_by(user_id: user_id) end def scrypt_password_digest(password:, salt:, cost:) @@ -63,11 +58,5 @@ def scrypt_password_digest(password:, salt:, cost:) scrypted = SCrypt::Engine.hash_secret password, scrypt_salt, 32 SCrypt::Password.new(scrypted).digest end - - private - - def create_fingerprint(code) - Pii::Fingerprinter.fingerprint(code) - end end end diff --git a/app/models/concerns/backup_code_encrypted_attribute_overrides.rb b/app/models/concerns/backup_code_encrypted_attribute_overrides.rb index 8a1109480f8..495b610fb59 100644 --- a/app/models/concerns/backup_code_encrypted_attribute_overrides.rb +++ b/app/models/concerns/backup_code_encrypted_attribute_overrides.rb @@ -2,15 +2,11 @@ module BackupCodeEncryptedAttributeOverrides extend ActiveSupport::Concern # Override ActiveModel::Dirty methods in order to - # use code_fingerprint_changed? instead of code_changed? + # use salted_code_fingerprint_changed? instead of code_changed? # This is necessary because code is no longer an ActiveRecord # attribute and all the *_changed and *_was magic no longer works. def will_save_change_to_code? - code_fingerprint_changed? - end - - def code_in_database - EncryptedAttribute.new(encrypted_code_was).decrypted if encrypted_code_was.present? + salted_code_fingerprint_changed? end # Override usual setter method in order to also set fingerprint @@ -24,13 +20,5 @@ def code=(code) cost: code_cost, ) end - - if skip_legacy_encryption? - self.encrypted_code = '' - self.code_fingerprint = self.salted_code_fingerprint # "garbage" value, has to be unique - else - set_encrypted_attribute(name: :code, value: code) - self.code_fingerprint = code.present? ? encrypted_attributes[:code].fingerprint : '' - end end end diff --git a/app/services/backup_code_benchmarker.rb b/app/services/backup_code_benchmarker.rb deleted file mode 100644 index 1f3fc993ec7..00000000000 --- a/app/services/backup_code_benchmarker.rb +++ /dev/null @@ -1,94 +0,0 @@ -# This is for benchmarking backup code conversion to salted hashes -# DO NOT RUN IT IN PRODUCTION -class BackupCodeBenchmarker - # attribute_cost: "4000$8$4$" - # scrypt_cost: "10000$8$1$" - - attr_reader :cost - attr_reader :batch_size - attr_reader :num_rows - attr_reader :num_per_user - attr_reader :logger - - def initialize( - cost: '10000$8$1$', - batch_size: 1000, - num_rows: 100_000, # number of rows to test backfilling - num_per_user: BackupCodeGenerator::NUMBER_OF_CODES, # defaults to 10 - logger: Logger.new(STDOUT) - ) - @cost = cost - @batch_size = batch_size - @num_rows = num_rows - @num_per_user = num_per_user - @logger = logger - end - - # @yield block to call after rows are created, before they are backfilled - def run(&before_convert) - raise 'do not run in prod' if Identity::Hostdata.env == 'prod' - - silence_active_record_logger do - prepare! - yield if before_convert - convert! - end - end - - def prepare! - return if BackupCodeConfiguration.count >= num_rows - - user_id = User.first&.id || 1 - num_to_create = num_rows - BackupCodeConfiguration.count - - generator = BackupCodeGenerator.new(nil) - - logger.info "creating #{num_to_create} backup code configurations" - - num_to_create.times.each_slice(batch_size) do |slice| - slice.each_slice(num_per_user) do |user_slice| - user_id += 1 - - user_slice.each do - code = generator.send(:backup_code) - - BackupCodeConfiguration.create(user_id: user_id, code: code) - end - end - end - - logger.info 'done creating backup codes' - end - - def convert! - job = BackupCodeBackfillerJob.new - - Benchmark.realtime do - BackupCodeConfiguration.limit(num_rows).find_in_batches(batch_size: batch_size) do |batch| - Benchmark.realtime do - batch.each_slice(num_per_user) do |slice| - Benchmark.realtime do - job.perform_batch(slice) - end.tap do |duration| - logger.info "duration=#{duration} batch_size=#{slice.size}" - end - end - end.tap do |duration| - logger.info "duration=#{duration} batch_size=#{batch.size}" - end - end - end.tap do |duration| - logger.info "duration=#{duration} batch_size=#{num_rows} (done)" - end - end - - # @yield a block to run with a silenced AR logger - def silence_active_record_logger - ar_logger = ActiveRecord::Base.logger - ActiveRecord::Base.logger = nil - - yield - ensure - ActiveRecord::Base.logger = ar_logger - end -end diff --git a/app/services/backup_code_generator.rb b/app/services/backup_code_generator.rb index d86a16d5eb7..52bead375f3 100644 --- a/app/services/backup_code_generator.rb +++ b/app/services/backup_code_generator.rb @@ -5,18 +5,9 @@ class BackupCodeGenerator NUMBER_OF_CODES = 10 - def initialize( - user, - num_words: BackupCodeConfiguration::NUM_WORDS, - skip_legacy_encryption: IdentityConfig.store.backup_code_skip_symmetric_encryption - ) + def initialize(user, num_words: BackupCodeConfiguration::NUM_WORDS) @num_words = num_words @user = user - @skip_legacy_encryption = skip_legacy_encryption - end - - def skip_legacy_encryption? - @skip_legacy_encryption end # @return [Array] @@ -68,7 +59,6 @@ def save_code(code:, salt:) @user.backup_code_configurations.create!( code_salt: salt, code_cost: cost, - skip_legacy_encryption: skip_legacy_encryption?, code: code, ) end diff --git a/config/application.yml.default b/config/application.yml.default index 9a5ce741eaf..a31fea7894d 100644 --- a/config/application.yml.default +++ b/config/application.yml.default @@ -47,7 +47,6 @@ aws_logo_bucket: '' aws_region: 'us-west-2' aws_kms_multi_region_enabled: 'false' backup_code_cost: '2000$8$1$' -backup_code_skip_symmetric_encryption: 'false' country_phone_number_overrides: '{}' doc_auth_error_dpi_threshold: '290' doc_auth_error_sharpness_threshold: '40' diff --git a/lib/identity_config.rb b/lib/identity_config.rb index 4203329f3bf..7892f1d3640 100644 --- a/lib/identity_config.rb +++ b/lib/identity_config.rb @@ -101,7 +101,6 @@ def self.build_store(config_map) config.add(:aws_logo_bucket, type: :string) config.add(:aws_region, type: :string) config.add(:backup_code_cost, type: :string) - config.add(:backup_code_skip_symmetric_encryption, type: :boolean) config.add(:country_phone_number_overrides, type: :json) config.add(:dashboard_api_token, type: :string) config.add(:dashboard_url, type: :string) diff --git a/spec/jobs/backup_code_backfiller_job_spec.rb b/spec/jobs/backup_code_backfiller_job_spec.rb deleted file mode 100644 index feff94beddf..00000000000 --- a/spec/jobs/backup_code_backfiller_job_spec.rb +++ /dev/null @@ -1,49 +0,0 @@ -require 'rails_helper' - -RSpec.describe BackupCodeBackfillerJob do - describe '.enqueue_all' do - before do - User.delete_all - end - - let!(:users_with_legacy_encryption) do - 3.times.map do - user = create(:user) - codes = BackupCodeGenerator.new(user, skip_legacy_encryption: false).create - OpenStruct.new(user: user, codes: codes) - end - end - - let!(:users_without_legacy_encryption) do - 2.times.map do - user = create(:user) - codes = BackupCodeGenerator.new(user, skip_legacy_encryption: true).create - OpenStruct.new(user: user, codes: codes) - end - end - - it 'removes legacy encryption, batches salt by user and preserves the findability of codes' do - users_with_legacy_encryption.each do |user_codes| - expect(user_codes.user.backup_code_configurations.map(&:encrypted_code)).to all(be_present) - end - - BackupCodeBackfillerJob.enqueue_all - - (users_with_legacy_encryption + users_without_legacy_encryption).each do |user_codes| - user = user_codes.user.reload - - expect(user.backup_code_configurations.map(&:encrypted_code)).to all(be_blank) - - salt = user.backup_code_configurations.first.code_salt - expect(user.backup_code_configurations.map(&:code_salt)).to all(eq(salt)) - - code = BackupCodeConfiguration.find_with_code( - code: user_codes.codes.first, - user_id: user.id, - ) - - expect(code).to be_present - end - end - end -end diff --git a/spec/models/backup_code_configuration_spec.rb b/spec/models/backup_code_configuration_spec.rb index d99a69d1825..130ed2c3a48 100644 --- a/spec/models/backup_code_configuration_spec.rb +++ b/spec/models/backup_code_configuration_spec.rb @@ -47,14 +47,6 @@ end end - describe 'code_in_database' do - it 'returns nil' do - backup_code_config = BackupCodeConfiguration.new - - expect(backup_code_config.code_in_database).to eq nil - end - end - describe 'will_save_change_to_code?' do it 'returns false if code did not change' do backup_code_config = BackupCodeConfiguration.new @@ -64,6 +56,8 @@ it 'returns true if code changed' do backup_code_config = BackupCodeConfiguration.new + backup_code_config.code_cost = IdentityConfig.store.backup_code_cost + backup_code_config.code_salt = 'aaa' backup_code_config.code = 'foo' expect(backup_code_config.will_save_change_to_code?).to eq true @@ -87,28 +81,10 @@ expect(BackupCodeConfiguration.find_with_code(code: first_code, user_id: 1234)).to be_nil end - it 'finds codes via code_fingerprint' do - codes = BackupCodeGenerator.new(user, skip_legacy_encryption: false).create - first_code = codes.first - - # overwrite with a wrong value so queries use the other column - BackupCodeConfiguration.all.each_with_index do |code, index| - code.update!(salted_code_fingerprint: index) - end - - backup_code = BackupCodeConfiguration.find_with_code(code: first_code, user_id: user.id) - expect(backup_code).to be - end - it 'finds codes via salted_code_fingerprint' do codes = BackupCodeGenerator.new(user).create first_code = codes.first - # overwrite with a wrong value so queries use the other column - BackupCodeConfiguration.all.each_with_index do |code, index| - code.update!(code_fingerprint: index) - end - backup_code = BackupCodeConfiguration.find_with_code(code: first_code, user_id: user.id) expect(backup_code).to be end @@ -137,9 +113,7 @@ def save_and_find(find:, save: 'just-some-not-null-value', fingerprint: nil) code_cost: '10$8$4$', code_salt: 'abcdefg', code: save, - ).tap do |config| - config.code_fingerprint = fingerprint if fingerprint - end.save! + ).save! BackupCodeConfiguration.find_with_code(code: find, user_id: user.id) end @@ -151,9 +125,6 @@ def save_and_find(find:, save: 'just-some-not-null-value', fingerprint: nil) it 'finds codes if they were generated the old way (with SecureRandom.hex)' do code = SecureRandom.hex(3 * 4 / 2) expect(save_and_find(save: code, find: code)).to be - - code = SecureRandom.hex(3 * 4 / 2) - expect(save_and_find(fingerprint: Pii::Fingerprinter.fingerprint(code), find: code)).to be end end diff --git a/spec/services/backup_code_benchmarker_spec.rb b/spec/services/backup_code_benchmarker_spec.rb deleted file mode 100644 index 0058888373e..00000000000 --- a/spec/services/backup_code_benchmarker_spec.rb +++ /dev/null @@ -1,69 +0,0 @@ -require 'rails_helper' - -RSpec.describe BackupCodeBenchmarker do - let(:logger) { Logger.new('/dev/null') } - - let(:num_rows) { 4 } - let(:num_per_user) { 2 } - let(:user) { build(:user) } - subject(:benchmarker) do - BackupCodeBenchmarker.new( - cost: '10$8$4$', - logger: logger, - num_rows: num_rows, - num_per_user: num_per_user, - batch_size: 10, - ) - end - - describe '#run' do - context 'in production' do - before do - expect(Identity::Hostdata).to receive(:env).and_return('prod') - end - - it 'bails and does not run' do - expect { benchmarker.run }.to raise_error('do not run in prod') - end - end - - context 'when enough backup code configurations already exist' do - let(:num_rows) { 2 } - - before do - num_rows.times { create(:backup_code_configuration, user: user) } - end - - it 'does not create any new ones' do - expect { benchmarker.run }.to_not(change { BackupCodeConfiguration.count }) - end - end - - it 'sets scrypted value' do - id_to_code = nil - - benchmarker.run do - id_to_code = BackupCodeConfiguration.all.map { |b| [b.id, b.code] }.to_h - end - - expect(BackupCodeConfiguration.count).to eq(num_rows) - - BackupCodeConfiguration.all.each do |cfg| - expect(cfg.salted_code_fingerprint).to eq( - BackupCodeConfiguration.scrypt_password_digest( - password: id_to_code.fetch(cfg.id), - salt: cfg.code_salt, - cost: cfg.code_cost, - ), - ) - end - end - - it 'does not update beyond num_rows' do - (num_rows + 1).times { create(:backup_code_configuration, user: user) } - - expect { benchmarker.run }. - to_not(change { BackupCodeConfiguration.last.salted_code_fingerprint }) - end - end -end diff --git a/spec/services/backup_code_generator_spec.rb b/spec/services/backup_code_generator_spec.rb index a7f1b1225a7..294e099d281 100644 --- a/spec/services/backup_code_generator_spec.rb +++ b/spec/services/backup_code_generator_spec.rb @@ -31,39 +31,6 @@ expect(success).to be_falsy end - context 'backup_code_skip_symmetric_encryption config' do - before do - allow(IdentityConfig.store).to receive(:backup_code_skip_symmetric_encryption). - and_return(skip_symmetric) - end - - context 'when enabled' do - let(:skip_symmetric) { true } - - it 'does not write the symmetrically encrypted column anymore' do - codes = generator.create - - user.backup_code_configurations.each do |code_config| - expect(code_config.encrypted_code).to be_blank - expect(code_config.code).to be_blank - end - end - end - - context 'when disabled' do - let(:skip_symmetric) { false } - - it 'still writes the symmetrically encrypted column anymore' do - codes = generator.create - - user.backup_code_configurations.each do |code_config| - expect(code_config.encrypted_code).to be_present - expect(code_config.code).to be_present - end - end - end - end - it 'creates codes with the same salt for that batch' do generator.create