From 1e7d5d2957678788fdea8ade77eced98848ff4ff Mon Sep 17 00:00:00 2001 From: Matt Jankowski Date: Thu, 2 May 2024 05:31:41 -0400 Subject: [PATCH] Update `devise-two-factor` to version 5.0.0 (#28325) Co-authored-by: Claire --- Gemfile | 2 +- Gemfile.lock | 8 +- app/models/concerns/legacy_otp_secret.rb | 77 +++++++++++++++++++ app/models/user.rb | 8 +- config/environments/development.rb | 3 +- config/environments/production.rb | 1 + config/environments/test.rb | 1 + .../20231210154528_add_otp_secret_to_user.rb | 7 ++ ...80905_migrate_devise_two_factor_secrets.rb | 39 ++++++++++ db/schema.rb | 1 + lib/tasks/tests.rake | 18 ++++- spec/models/user_spec.rb | 15 +++- 12 files changed, 162 insertions(+), 18 deletions(-) create mode 100644 app/models/concerns/legacy_otp_secret.rb create mode 100644 db/migrate/20231210154528_add_otp_secret_to_user.rb create mode 100644 db/post_migrate/20240307180905_migrate_devise_two_factor_secrets.rb diff --git a/Gemfile b/Gemfile index a10613b30b9..eb507e9d185 100644 --- a/Gemfile +++ b/Gemfile @@ -31,7 +31,7 @@ gem 'browser' gem 'charlock_holmes', '~> 0.7.7' gem 'chewy', '~> 7.3' gem 'devise', '~> 4.9' -gem 'devise-two-factor', '~> 4.1' +gem 'devise-two-factor' group :pam_authentication, optional: true do gem 'devise_pam_authenticatable2', '~> 9.2' diff --git a/Gemfile.lock b/Gemfile.lock index 3394930e0d1..a231785402e 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -97,8 +97,6 @@ GEM activerecord (>= 3.2, < 8.0) rake (>= 10.4, < 14.0) ast (2.4.2) - attr_encrypted (4.0.0) - encryptor (~> 3.0.0) attr_required (1.0.2) awrence (1.2.1) aws-eventstream (1.3.0) @@ -204,9 +202,8 @@ GEM railties (>= 4.1.0) responders warden (~> 1.2.3) - devise-two-factor (4.1.1) + devise-two-factor (5.0.0) activesupport (~> 7.0) - attr_encrypted (>= 1.3, < 5, != 2) devise (~> 4.0) railties (~> 7.0) rotp (~> 6.0) @@ -236,7 +233,6 @@ GEM htmlentities (~> 4.3.3) launchy (~> 2.1) mail (~> 2.7) - encryptor (3.0.0) erubi (1.12.0) et-orbi (1.2.11) tzinfo @@ -842,7 +838,7 @@ DEPENDENCIES database_cleaner-active_record debug (~> 1.8) devise (~> 4.9) - devise-two-factor (~> 4.1) + devise-two-factor devise_pam_authenticatable2 (~> 9.2) discard (~> 1.2) doorkeeper (~> 5.6) diff --git a/app/models/concerns/legacy_otp_secret.rb b/app/models/concerns/legacy_otp_secret.rb new file mode 100644 index 00000000000..466c4ec9bb3 --- /dev/null +++ b/app/models/concerns/legacy_otp_secret.rb @@ -0,0 +1,77 @@ +# frozen_string_literal: true + +# TODO: This file is here for legacy support during devise-two-factor upgrade. +# It should be removed after all records have been migrated. + +module LegacyOtpSecret + extend ActiveSupport::Concern + + private + + # Decrypt and return the `encrypted_otp_secret` attribute which was used in + # prior versions of devise-two-factor + # @return [String] The decrypted OTP secret + def legacy_otp_secret + return nil unless self[:encrypted_otp_secret] + return nil unless self.class.otp_secret_encryption_key + + hmac_iterations = 2000 # a default set by the Encryptor gem + key = self.class.otp_secret_encryption_key + salt = Base64.decode64(encrypted_otp_secret_salt) + iv = Base64.decode64(encrypted_otp_secret_iv) + + raw_cipher_text = Base64.decode64(encrypted_otp_secret) + # The last 16 bytes of the ciphertext are the authentication tag - we use + # Galois Counter Mode which is an authenticated encryption mode + cipher_text = raw_cipher_text[0..-17] + auth_tag = raw_cipher_text[-16..-1] # rubocop:disable Style/SlicingWithRange + + # this alrorithm lifted from + # https://github.com/attr-encrypted/encryptor/blob/master/lib/encryptor.rb#L54 + + # create an OpenSSL object which will decrypt the AES cipher with 256 bit + # keys in Galois Counter Mode (GCM). See + # https://ruby.github.io/openssl/OpenSSL/Cipher.html + cipher = OpenSSL::Cipher.new('aes-256-gcm') + + # tell the cipher we want to decrypt. Symmetric algorithms use a very + # similar process for encryption and decryption, hence the same object can + # do both. + cipher.decrypt + + # Use a Password-Based Key Derivation Function to generate the key actually + # used for encryptoin from the key we got as input. + cipher.key = OpenSSL::PKCS5.pbkdf2_hmac_sha1(key, salt, hmac_iterations, cipher.key_len) + + # set the Initialization Vector (IV) + cipher.iv = iv + + # The tag must be set after calling Cipher#decrypt, Cipher#key= and + # Cipher#iv=, but before calling Cipher#final. After all decryption is + # performed, the tag is verified automatically in the call to Cipher#final. + # + # If the auth_tag does not verify, then #final will raise OpenSSL::Cipher::CipherError + cipher.auth_tag = auth_tag + + # auth_data must be set after auth_tag has been set when decrypting See + # http://ruby-doc.org/stdlib-2.0.0/libdoc/openssl/rdoc/OpenSSL/Cipher.html#method-i-auth_data-3D + # we are not adding any authenticated data but OpenSSL docs say this should + # still be called. + cipher.auth_data = '' + + # #update is (somewhat confusingly named) the method which actually + # performs the decryption on the given chunk of data. Our OTP secret is + # short so we only need to call it once. + # + # It is very important that we call #final because: + # + # 1. The authentication tag is checked during the call to #final + # 2. Block based cipher modes (e.g. CBC) work on fixed size chunks. We need + # to call #final to get it to process the last chunk properly. The output + # of #final should be appended to the decrypted value. This isn't + # required for streaming cipher modes but including it is a best practice + # so that your code will continue to function correctly even if you later + # change to a block cipher mode. + cipher.update(cipher_text) + cipher.final + end +end diff --git a/app/models/user.rb b/app/models/user.rb index 584120cf2eb..8bc0b23ce84 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -39,6 +39,7 @@ # role_id :bigint(8) # settings :text # time_zone :string +# otp_secret :string # class User < ApplicationRecord @@ -72,6 +73,8 @@ class User < ApplicationRecord devise :two_factor_authenticatable, otp_secret_encryption_key: Rails.configuration.x.otp_secret + include LegacyOtpSecret # Must be after the above `devise` line in order to override the legacy method + devise :two_factor_backupable, otp_number_of_backup_codes: 10 @@ -131,11 +134,6 @@ class User < ApplicationRecord normalizes :time_zone, with: ->(time_zone) { ActiveSupport::TimeZone[time_zone].nil? ? nil : time_zone } normalizes :chosen_languages, with: ->(chosen_languages) { chosen_languages.compact_blank.presence } - # This avoids a deprecation warning from Rails 5.1 - # It seems possible that a future release of devise-two-factor will - # handle this itself, and this can be removed from our User class. - attribute :otp_secret - has_many :session_activations, dependent: :destroy delegate :can?, to: :role diff --git a/config/environments/development.rb b/config/environments/development.rb index a855f5a16be..a3254125c03 100644 --- a/config/environments/development.rb +++ b/config/environments/development.rb @@ -87,8 +87,7 @@ Rails.application.configure do # Otherwise, use letter_opener, which launches a browser window to view sent mail. config.action_mailer.delivery_method = ENV['HEROKU'] || ENV['VAGRANT'] || ENV['REMOTE_DEV'] ? :letter_opener_web : :letter_opener - # We provide a default secret for the development environment here. - # This value should not be used in production environments! + # TODO: Remove once devise-two-factor data migration complete config.x.otp_secret = ENV.fetch('OTP_SECRET', '1fc2b87989afa6351912abeebe31ffc5c476ead9bf8b3d74cbc4a302c7b69a45b40b1bbef3506ddad73e942e15ed5ca4b402bf9a66423626051104f4b5f05109') # Raise error when a before_action's only/except options reference missing actions diff --git a/config/environments/production.rb b/config/environments/production.rb index 49e02b53d39..6b1101ea1ba 100644 --- a/config/environments/production.rb +++ b/config/environments/production.rb @@ -157,6 +157,7 @@ Rails.application.configure do 'Referrer-Policy' => 'same-origin', } + # TODO: Remove once devise-two-factor data migration complete config.x.otp_secret = ENV.fetch('OTP_SECRET') # Enable DNS rebinding protection and other `Host` header attacks. diff --git a/config/environments/test.rb b/config/environments/test.rb index 13e19733807..49b0c1f3031 100644 --- a/config/environments/test.rb +++ b/config/environments/test.rb @@ -44,6 +44,7 @@ Rails.application.configure do # Print deprecation notices to the stderr. config.active_support.deprecation = :stderr + # TODO: Remove once devise-two-factor data migration complete config.x.otp_secret = '100c7faeef00caa29242f6b04156742bf76065771fd4117990c4282b8748ff3d99f8fdae97c982ab5bd2e6756a159121377cce4421f4a8ecd2d67bd7749a3fb4' # Generate random VAPID keys diff --git a/db/migrate/20231210154528_add_otp_secret_to_user.rb b/db/migrate/20231210154528_add_otp_secret_to_user.rb new file mode 100644 index 00000000000..b2ce0a4f79b --- /dev/null +++ b/db/migrate/20231210154528_add_otp_secret_to_user.rb @@ -0,0 +1,7 @@ +# frozen_string_literal: true + +class AddOtpSecretToUser < ActiveRecord::Migration[7.1] + def change + add_column :users, :otp_secret, :string + end +end diff --git a/db/post_migrate/20240307180905_migrate_devise_two_factor_secrets.rb b/db/post_migrate/20240307180905_migrate_devise_two_factor_secrets.rb new file mode 100644 index 00000000000..360e4806da2 --- /dev/null +++ b/db/post_migrate/20240307180905_migrate_devise_two_factor_secrets.rb @@ -0,0 +1,39 @@ +# frozen_string_literal: true + +class MigrateDeviseTwoFactorSecrets < ActiveRecord::Migration[7.1] + disable_ddl_transaction! + + class MigrationUser < ApplicationRecord + self.table_name = :users + + devise :two_factor_authenticatable, + otp_secret_encryption_key: Rails.configuration.x.otp_secret + + include LegacyOtpSecret # Must be after the above `devise` line in order to override the legacy method + end + + def up + MigrationUser.reset_column_information + + users_with_otp_enabled.find_each do |user| + # Gets the new value on already-updated users + # Falls back to legacy value on not-yet-migrated users + otp_secret = user.otp_secret + + Rails.logger.debug { "Processing #{user.email}" } + + # This is a no-op for migrated users and updates format for not migrated + user.update!(otp_secret: otp_secret) + end + end + + def down + raise ActiveRecord::IrreversibleMigration + end + + private + + def users_with_otp_enabled + MigrationUser.where(otp_required_for_login: true, otp_secret: nil) + end +end diff --git a/db/schema.rb b/db/schema.rb index a875c6ffc79..11f1a202f79 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -1199,6 +1199,7 @@ ActiveRecord::Schema[7.1].define(version: 2024_03_22_161611) do t.bigint "role_id" t.text "settings" t.string "time_zone" + t.string "otp_secret" t.index ["account_id"], name: "index_users_on_account_id" t.index ["confirmation_token"], name: "index_users_on_confirmation_token", unique: true t.index ["created_by_application_id"], name: "index_users_on_created_by_application_id", where: "(created_by_application_id IS NOT NULL)" diff --git a/lib/tasks/tests.rake b/lib/tasks/tests.rake index 0caebf92a18..c8e0312bbd8 100644 --- a/lib/tasks/tests.rake +++ b/lib/tasks/tests.rake @@ -127,6 +127,14 @@ namespace :tests do exit(1) end + # This is checking the attribute rather than the method, to avoid the legacy fallback + # and ensure the data has been migrated + unless Account.find_local('qcuser').user[:otp_secret] == 'anotpsecretthatshouldbeencrypted' + puts "DEBUG: #{Account.find_local('qcuser').user.inspect}" + puts 'OTP secret for user not preserved as expected' + exit(1) + end + puts 'No errors found. Database state is consistent with a successful migration process.' end @@ -213,9 +221,15 @@ namespace :tests do (4, 10, 'kmruser@localhost', now(), now(), false, 'ku', '{en,kmr,ku,ckb}'); INSERT INTO "users" - (id, account_id, email, created_at, updated_at, locale) + (id, account_id, email, created_at, updated_at, locale, + encrypted_otp_secret, encrypted_otp_secret_iv, encrypted_otp_secret_salt, + otp_required_for_login) VALUES - (5, 11, 'qcuser@localhost', now(), now(), 'fr-QC'); + (5, 11, 'qcuser@localhost', now(), now(), 'fr-QC', + E'Fttsy7QAa0edaDfdfSz094rRLAxc8cJweDQ4BsWH/zozcdVA8o9GLqcKhn2b\nGi/V\n', + 'rys3THICkr60BoWC', + '_LMkAGvdg7a+sDIKjI3mR2Q==', + true); INSERT INTO "settings" (id, thing_type, thing_id, var, value, created_at, updated_at) diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 714d595dc12..fa0a0503a65 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -9,14 +9,25 @@ RSpec.describe User do it_behaves_like 'two_factor_backupable' - describe 'otp_secret' do + describe 'legacy_otp_secret' do it 'is encrypted with OTP_SECRET environment variable' do user = Fabricate(:user, encrypted_otp_secret: "Fttsy7QAa0edaDfdfSz094rRLAxc8cJweDQ4BsWH/zozcdVA8o9GLqcKhn2b\nGi/V\n", encrypted_otp_secret_iv: 'rys3THICkr60BoWC', encrypted_otp_secret_salt: '_LMkAGvdg7a+sDIKjI3mR2Q==') - expect(user.otp_secret).to eq 'anotpsecretthatshouldbeencrypted' + expect(user.send(:legacy_otp_secret)).to eq 'anotpsecretthatshouldbeencrypted' + end + end + + describe 'otp_secret' do + it 'encrypts the saved value' do + user = Fabricate(:user, otp_secret: '123123123') + + user.reload + + expect(user.otp_secret).to eq '123123123' + expect(user.attributes_before_type_cast[:otp_secret]).to_not eq '123123123' end end