From 95ec60e3f4a00c413fa06e6cc86a575c9d72039e Mon Sep 17 00:00:00 2001 From: Matt Jankowski Date: Thu, 2 May 2024 15:43:15 -0400 Subject: [PATCH 1/3] Remove Legacy OTP Secret code --- app/models/concerns/legacy_otp_secret.rb | 77 ------------------- app/models/user.rb | 15 ++-- ...cy_devise_two_factor_secrets_from_users.rb | 11 +++ ...80905_migrate_devise_two_factor_secrets.rb | 75 +++++++++++++++++- db/schema.rb | 5 +- spec/models/user_spec.rb | 11 --- 6 files changed, 94 insertions(+), 100 deletions(-) delete mode 100644 app/models/concerns/legacy_otp_secret.rb create mode 100644 db/migrate/20240502192024_remove_legacy_devise_two_factor_secrets_from_users.rb diff --git a/app/models/concerns/legacy_otp_secret.rb b/app/models/concerns/legacy_otp_secret.rb deleted file mode 100644 index 466c4ec9bb3..00000000000 --- a/app/models/concerns/legacy_otp_secret.rb +++ /dev/null @@ -1,77 +0,0 @@ -# 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 8bc0b23ce84..905a54834c3 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -44,14 +44,17 @@ class User < ApplicationRecord self.ignored_columns += %w( + admin + current_sign_in_ip + encrypted_otp_secret + encrypted_otp_secret_iv + encrypted_otp_secret_salt + filtered_languages + last_sign_in_ip + moderator remember_created_at remember_token - current_sign_in_ip - last_sign_in_ip skip_sign_in_token - filtered_languages - admin - moderator ) include LanguagesHelper @@ -73,8 +76,6 @@ 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 diff --git a/db/migrate/20240502192024_remove_legacy_devise_two_factor_secrets_from_users.rb b/db/migrate/20240502192024_remove_legacy_devise_two_factor_secrets_from_users.rb new file mode 100644 index 00000000000..b5a4cae5926 --- /dev/null +++ b/db/migrate/20240502192024_remove_legacy_devise_two_factor_secrets_from_users.rb @@ -0,0 +1,11 @@ +# frozen_string_literal: true + +class RemoveLegacyDeviseTwoFactorSecretsFromUsers < ActiveRecord::Migration[7.1] + def change + safety_assured do + remove_column :users, :encrypted_otp_secret + remove_column :users, :encrypted_otp_secret_iv + remove_column :users, :encrypted_otp_secret_salt + end + 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 index 360e4806da2..b670f670200 100644 --- a/db/post_migrate/20240307180905_migrate_devise_two_factor_secrets.rb +++ b/db/post_migrate/20240307180905_migrate_devise_two_factor_secrets.rb @@ -3,13 +3,86 @@ class MigrateDeviseTwoFactorSecrets < ActiveRecord::Migration[7.1] disable_ddl_transaction! + 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 + 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 + include LegacyOtpSecret end def up diff --git a/db/schema.rb b/db/schema.rb index 11f1a202f79..f8a9d5b0617 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[7.1].define(version: 2024_03_22_161611) do +ActiveRecord::Schema[7.1].define(version: 2024_05_02_192024) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -1178,9 +1178,6 @@ ActiveRecord::Schema[7.1].define(version: 2024_03_22_161611) do t.datetime "confirmation_sent_at", precision: nil t.string "unconfirmed_email" t.string "locale" - t.string "encrypted_otp_secret" - t.string "encrypted_otp_secret_iv" - t.string "encrypted_otp_secret_salt" t.integer "consumed_timestep" t.boolean "otp_required_for_login", default: false, null: false t.datetime "last_emailed_at", precision: nil diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index fa0a0503a65..04bc4603b2e 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -9,17 +9,6 @@ RSpec.describe User do it_behaves_like 'two_factor_backupable' - 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.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') From ff0926e078245ef0f7712b33c88752e9852378ff Mon Sep 17 00:00:00 2001 From: Matt Jankowski Date: Thu, 2 May 2024 16:17:45 -0400 Subject: [PATCH 2/3] Move to post migrate --- ...ove_legacy_devise_two_factor_secrets_from_users.rb | 11 ----------- ...ove_legacy_devise_two_factor_secrets_from_users.rb | 11 +++++++++++ 2 files changed, 11 insertions(+), 11 deletions(-) delete mode 100644 db/migrate/20240502192024_remove_legacy_devise_two_factor_secrets_from_users.rb create mode 100644 db/post_migrate/20240502192024_remove_legacy_devise_two_factor_secrets_from_users.rb diff --git a/db/migrate/20240502192024_remove_legacy_devise_two_factor_secrets_from_users.rb b/db/migrate/20240502192024_remove_legacy_devise_two_factor_secrets_from_users.rb deleted file mode 100644 index b5a4cae5926..00000000000 --- a/db/migrate/20240502192024_remove_legacy_devise_two_factor_secrets_from_users.rb +++ /dev/null @@ -1,11 +0,0 @@ -# frozen_string_literal: true - -class RemoveLegacyDeviseTwoFactorSecretsFromUsers < ActiveRecord::Migration[7.1] - def change - safety_assured do - remove_column :users, :encrypted_otp_secret - remove_column :users, :encrypted_otp_secret_iv - remove_column :users, :encrypted_otp_secret_salt - end - end -end diff --git a/db/post_migrate/20240502192024_remove_legacy_devise_two_factor_secrets_from_users.rb b/db/post_migrate/20240502192024_remove_legacy_devise_two_factor_secrets_from_users.rb new file mode 100644 index 00000000000..b883bd2b28d --- /dev/null +++ b/db/post_migrate/20240502192024_remove_legacy_devise_two_factor_secrets_from_users.rb @@ -0,0 +1,11 @@ +# frozen_string_literal: true + +class RemoveLegacyDeviseTwoFactorSecretsFromUsers < ActiveRecord::Migration[7.1] + def change + safety_assured do + remove_column :users, :encrypted_otp_secret, :string + remove_column :users, :encrypted_otp_secret_iv, :string + remove_column :users, :encrypted_otp_secret_salt, :string + end + end +end From 419e8576bf23e0827fa0a1c317111982868d119a Mon Sep 17 00:00:00 2001 From: Matt Jankowski Date: Thu, 2 May 2024 16:18:56 -0400 Subject: [PATCH 3/3] Annotate update --- app/models/user.rb | 3 --- 1 file changed, 3 deletions(-) diff --git a/app/models/user.rb b/app/models/user.rb index 905a54834c3..af19f93796e 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -19,9 +19,6 @@ # confirmation_sent_at :datetime # unconfirmed_email :string # locale :string -# encrypted_otp_secret :string -# encrypted_otp_secret_iv :string -# encrypted_otp_secret_salt :string # consumed_timestep :integer # otp_required_for_login :boolean default(FALSE), not null # last_emailed_at :datetime