From baa2b9f7cf41a79310991a62f2e8b298b3c20031 Mon Sep 17 00:00:00 2001 From: Gabriel Sobrinho Date: Thu, 30 Apr 2020 20:00:50 -0300 Subject: [PATCH 1/5] Add rolling option --- Changelog.md | 1 + lib/devise/encryptable/encryptable.rb | 4 ++ lib/devise/encryptable/model.rb | 54 +++++++++++++++++---- test/devise/encryptable/encryptable_test.rb | 15 ++++++ test/support/swappers.rb | 1 + 5 files changed, 65 insertions(+), 10 deletions(-) diff --git a/Changelog.md b/Changelog.md index b432c8c..7bdac78 100644 --- a/Changelog.md +++ b/Changelog.md @@ -2,6 +2,7 @@ * Support Rails 4.1 - 6.1, dropped support for Rails <= 4.0. (same support as Devise as of v4.8) * Support Ruby 2.1 - 3.0, dropped support for Ruby <= 2.0. (same support as Devise as of v4.8) +* Add support to roll legacy encryptors ### 0.2.0 diff --git a/lib/devise/encryptable/encryptable.rb b/lib/devise/encryptable/encryptable.rb index cfd5af3..e5d6afd 100644 --- a/lib/devise/encryptable/encryptable.rb +++ b/lib/devise/encryptable/encryptable.rb @@ -13,6 +13,10 @@ module Devise mattr_accessor :encryptor @@encryptor = nil + # Used to define a legacy algorithm that needs to be rolled to a new one. + mattr_accessor :transition_from_encryptor + @@transition_from_encryptor = nil + module Encryptable module Encryptors autoload :AuthlogicSha512, 'devise/encryptable/encryptors/authlogic_sha512' diff --git a/lib/devise/encryptable/model.rb b/lib/devise/encryptable/model.rb index c84beca..b23f552 100644 --- a/lib/devise/encryptable/model.rb +++ b/lib/devise/encryptable/model.rb @@ -13,9 +13,13 @@ module Models # # * +encryptor+: the encryptor going to be used. By default is nil. # + # * +transition_from_encryptor+: the legacy encryptor that needs to rolled to the +encryptor+. + # # == Examples # - # User.find(1).valid_password?('password123') # returns true/false + # User.find(1).valid_password?('password123') + # #=> returns true/false + # #=> if true and using a legacy encryptor, it will update the user to the new encryptor. # module Encryptable extend ActiveSupport::Concern @@ -38,7 +42,21 @@ def password=(new_password) # Validates the password considering the salt. def valid_password?(password) return false if encrypted_password.blank? - encryptor_class.compare(encrypted_password, password, self.class.stretches, authenticatable_salt, self.class.pepper) + + encryptor_arguments = [ + encrypted_password, + password, + self.class.stretches, + authenticatable_salt, + self.class.pepper + ] + + if transition_from_encryptor_class.try(:compare, *encryptor_arguments) + update_attribute(:password, password) + return true + end + + encryptor_class.compare(*encryptor_arguments) end # Overrides authenticatable salt to use the new password_salt @@ -62,24 +80,40 @@ def encryptor_class self.class.encryptor_class end + def transition_from_encryptor_class + self.class.transition_from_encryptor_class + end + module ClassMethods Devise::Models.config(self, :encryptor) + Devise::Models.config(self, :transition_from_encryptor) # Returns the class for the configured encryptor. def encryptor_class - @encryptor_class ||= case encryptor - when :bcrypt - raise "In order to use bcrypt as encryptor, simply remove :encryptable from your devise model" - when nil - raise "You need to give an :encryptor as option in order to use :encryptable" - else - Devise::Encryptable::Encryptors.const_get(encryptor.to_s.classify) - end + @encryptor_class ||= compute_encryptor_class(encryptor) + end + + # Returns the class for the configured transition from encryptor. + def transition_from_encryptor_class + @transition_from_encryptor_class ||= compute_encryptor_class(transition_from_encryptor) if transition_from_encryptor end def password_salt self.encryptor_class.salt(self.stretches) end + + private + + def compute_encryptor_class(encryptor) + case encryptor + when :bcrypt + raise "In order to use bcrypt as encryptor, simply remove :encryptable from your devise model" + when nil + raise "You need to give an :encryptor as option in order to use :encryptable" + else + Devise::Encryptable::Encryptors.const_get(encryptor.to_s.classify) + end + end end end end diff --git a/test/devise/encryptable/encryptable_test.rb b/test/devise/encryptable/encryptable_test.rb index 68bb125..3e474d6 100644 --- a/test/devise/encryptable/encryptable_test.rb +++ b/test/devise/encryptable/encryptable_test.rb @@ -48,6 +48,21 @@ def encrypt_password(admin, pepper=Admin.pepper, stretches=Admin.stretches, encr assert_not_equal encrypted_password, admin.encrypted_password end + test 'should roll from legacy encryptor to the new one' do + admin = nil + + swap_with_encryptor Admin, :sha1 do + admin = create_admin + assert admin.valid_password?('123456') + assert_equal admin.encrypted_password, encrypt_password(admin, Admin.pepper, Admin.stretches, Devise::Encryptable::Encryptors::Sha1) + end + + swap_with_encryptor Admin, :sha512, transition_from_encryptor: :sha1 do + assert admin.valid_password?('123456') + assert_equal admin.encrypted_password, encrypt_password(admin, Admin.pepper, Admin.stretches, Devise::Encryptable::Encryptors::Sha512) + end + end + test 'should respect encryptor configuration' do swap_with_encryptor Admin, :sha512 do admin = create_admin diff --git a/test/support/swappers.rb b/test/support/swappers.rb index a2d8715..acabca6 100644 --- a/test/support/swappers.rb +++ b/test/support/swappers.rb @@ -2,6 +2,7 @@ module Support module Swappers def swap_with_encryptor(klass, encryptor, options={}) klass.instance_variable_set(:@encryptor_class, nil) + klass.instance_variable_set(:@transition_from_encryptor, nil) swap klass, options.merge(:encryptor => encryptor) do begin From 81fbeb3cb802c52b9c3397b3c51de08a71d8a28d Mon Sep 17 00:00:00 2001 From: Gabriel Sobrinho Date: Mon, 24 Jan 2022 16:00:08 -0300 Subject: [PATCH 2/5] Fix CI --- .gitignore | 1 + Gemfile.lock | 163 --------------------------------------------------- 2 files changed, 1 insertion(+), 163 deletions(-) delete mode 100644 Gemfile.lock diff --git a/.gitignore b/.gitignore index bf7c7ba..bc3f3e9 100644 --- a/.gitignore +++ b/.gitignore @@ -16,3 +16,4 @@ test/version_tmp tmp *.sqlite gemfiles/*.lock +Gemfile.lock diff --git a/Gemfile.lock b/Gemfile.lock deleted file mode 100644 index 5aec86d..0000000 --- a/Gemfile.lock +++ /dev/null @@ -1,163 +0,0 @@ -PATH - remote: . - specs: - devise-encryptable (0.2.0) - devise (>= 2.1.0) - -GEM - remote: https://rubygems.org/ - specs: - actioncable (6.1.3.2) - actionpack (= 6.1.3.2) - activesupport (= 6.1.3.2) - nio4r (~> 2.0) - websocket-driver (>= 0.6.1) - actionmailbox (6.1.3.2) - actionpack (= 6.1.3.2) - activejob (= 6.1.3.2) - activerecord (= 6.1.3.2) - activestorage (= 6.1.3.2) - activesupport (= 6.1.3.2) - mail (>= 2.7.1) - actionmailer (6.1.3.2) - actionpack (= 6.1.3.2) - actionview (= 6.1.3.2) - activejob (= 6.1.3.2) - activesupport (= 6.1.3.2) - mail (~> 2.5, >= 2.5.4) - rails-dom-testing (~> 2.0) - actionpack (6.1.3.2) - actionview (= 6.1.3.2) - activesupport (= 6.1.3.2) - rack (~> 2.0, >= 2.0.9) - rack-test (>= 0.6.3) - rails-dom-testing (~> 2.0) - rails-html-sanitizer (~> 1.0, >= 1.2.0) - actiontext (6.1.3.2) - actionpack (= 6.1.3.2) - activerecord (= 6.1.3.2) - activestorage (= 6.1.3.2) - activesupport (= 6.1.3.2) - nokogiri (>= 1.8.5) - actionview (6.1.3.2) - activesupport (= 6.1.3.2) - builder (~> 3.1) - erubi (~> 1.4) - rails-dom-testing (~> 2.0) - rails-html-sanitizer (~> 1.1, >= 1.2.0) - activejob (6.1.3.2) - activesupport (= 6.1.3.2) - globalid (>= 0.3.6) - activemodel (6.1.3.2) - activesupport (= 6.1.3.2) - activerecord (6.1.3.2) - activemodel (= 6.1.3.2) - activesupport (= 6.1.3.2) - activestorage (6.1.3.2) - actionpack (= 6.1.3.2) - activejob (= 6.1.3.2) - activerecord (= 6.1.3.2) - activesupport (= 6.1.3.2) - marcel (~> 1.0.0) - mini_mime (~> 1.0.2) - activesupport (6.1.3.2) - concurrent-ruby (~> 1.0, >= 1.0.2) - i18n (>= 1.6, < 2) - minitest (>= 5.1) - tzinfo (~> 2.0) - zeitwerk (~> 2.3) - bcrypt (3.1.16) - builder (3.2.4) - concurrent-ruby (1.1.8) - crass (1.0.6) - devise (4.8.0) - bcrypt (~> 3.0) - orm_adapter (~> 0.1) - railties (>= 4.1.0) - responders - warden (~> 1.2.3) - erubi (1.10.0) - globalid (0.4.2) - activesupport (>= 4.2.0) - i18n (1.8.10) - concurrent-ruby (~> 1.0) - loofah (2.9.1) - crass (~> 1.0.2) - nokogiri (>= 1.5.9) - mail (2.7.1) - mini_mime (>= 0.1.1) - marcel (1.0.1) - method_source (1.0.0) - mini_mime (1.0.3) - mini_portile2 (2.5.3) - minitest (5.14.4) - mocha (1.12.0) - nio4r (2.5.7) - nokogiri (1.11.7) - mini_portile2 (~> 2.5.0) - racc (~> 1.4) - orm_adapter (0.5.0) - racc (1.5.2) - rack (2.2.3) - rack-test (1.1.0) - rack (>= 1.0, < 3) - rails (6.1.3.2) - actioncable (= 6.1.3.2) - actionmailbox (= 6.1.3.2) - actionmailer (= 6.1.3.2) - actionpack (= 6.1.3.2) - actiontext (= 6.1.3.2) - actionview (= 6.1.3.2) - activejob (= 6.1.3.2) - activemodel (= 6.1.3.2) - activerecord (= 6.1.3.2) - activestorage (= 6.1.3.2) - activesupport (= 6.1.3.2) - bundler (>= 1.15.0) - railties (= 6.1.3.2) - sprockets-rails (>= 2.0.0) - rails-dom-testing (2.0.3) - activesupport (>= 4.2.0) - nokogiri (>= 1.6) - rails-html-sanitizer (1.3.0) - loofah (~> 2.3) - railties (6.1.3.2) - actionpack (= 6.1.3.2) - activesupport (= 6.1.3.2) - method_source - rake (>= 0.8.7) - thor (~> 1.0) - rake (13.0.3) - responders (3.0.1) - actionpack (>= 5.0) - railties (>= 5.0) - sprockets (4.0.2) - concurrent-ruby (~> 1.0) - rack (> 1, < 3) - sprockets-rails (3.2.2) - actionpack (>= 4.0) - activesupport (>= 4.0) - sprockets (>= 3.0.0) - sqlite3 (1.4.2) - thor (1.1.0) - tzinfo (2.0.4) - concurrent-ruby (~> 1.0) - warden (1.2.9) - rack (>= 2.0.9) - websocket-driver (0.7.4) - websocket-extensions (>= 0.1.0) - websocket-extensions (0.1.5) - zeitwerk (2.4.2) - -PLATFORMS - ruby - -DEPENDENCIES - devise (~> 4.8) - devise-encryptable! - mocha (~> 1.0) - rails (~> 6.1.0) - sqlite3 - -BUNDLED WITH - 2.2.7 From 483034bb627036c850b92fb9fa4e8e14cb7c97fd Mon Sep 17 00:00:00 2001 From: Jeff Trudeau Date: Fri, 21 Aug 2020 15:20:13 -0400 Subject: [PATCH 3/5] Support PBKDF2 using SHA512 --- lib/devise/encryptable/encryptable.rb | 3 ++- lib/devise/encryptable/encryptors/pbkdf2.rb | 25 +++++++++++++++++++++ 2 files changed, 27 insertions(+), 1 deletion(-) create mode 100644 lib/devise/encryptable/encryptors/pbkdf2.rb diff --git a/lib/devise/encryptable/encryptable.rb b/lib/devise/encryptable/encryptable.rb index cfd5af3..1a39b80 100644 --- a/lib/devise/encryptable/encryptable.rb +++ b/lib/devise/encryptable/encryptable.rb @@ -18,6 +18,7 @@ module Encryptors autoload :AuthlogicSha512, 'devise/encryptable/encryptors/authlogic_sha512' autoload :Base, 'devise/encryptable/encryptors/base' autoload :ClearanceSha1, 'devise/encryptable/encryptors/clearance_sha1' + autoload :Pbkdf2, 'devise/encryptable/encryptors/pbkdf2' autoload :RestfulAuthenticationSha1, 'devise/encryptable/encryptors/restful_authentication_sha1' autoload :Sha1, 'devise/encryptable/encryptors/sha1' autoload :Sha512, 'devise/encryptable/encryptors/sha512' @@ -25,4 +26,4 @@ module Encryptors end end -Devise.add_module(:encryptable, :model => 'devise/encryptable/model') \ No newline at end of file +Devise.add_module(:encryptable, :model => 'devise/encryptable/model') diff --git a/lib/devise/encryptable/encryptors/pbkdf2.rb b/lib/devise/encryptable/encryptors/pbkdf2.rb new file mode 100644 index 0000000..c06d890 --- /dev/null +++ b/lib/devise/encryptable/encryptors/pbkdf2.rb @@ -0,0 +1,25 @@ +begin + module Devise + module Encryptable + module Encryptors + class Pbkdf2 < Base + def self.compare(encrypted_password, password, stretches, salt, pepper) + value_to_test = self.digest(password, stretches, salt, pepper) + ActiveSupport::SecurityUtils.fixed_length_secure_compare(encrypted_password, value_to_test) + end + + def self.digest(password, stretches, salt, pepper) + hash = OpenSSL::Digest::SHA512.new + OpenSSL::KDF.pbkdf2_hmac( + password, + salt: "#{[salt].pack('H*')}#{pepper}", + iterations: stretches, + hash: hash, + length: hash.digest_length, + ).unpack('H*')[0] + end + end + end + end + end +end From 0bc8ea7db9fe08f7c81a7e83424f6dff2a587ba6 Mon Sep 17 00:00:00 2001 From: Jeff Trudeau Date: Thu, 16 Jun 2022 15:04:15 -0400 Subject: [PATCH 4/5] Changes per https://github.com/heartcombo/devise-encryptable/pull/21 --- lib/devise/encryptable/encryptors/pbkdf2.rb | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/devise/encryptable/encryptors/pbkdf2.rb b/lib/devise/encryptable/encryptors/pbkdf2.rb index c06d890..cf0a3d8 100644 --- a/lib/devise/encryptable/encryptors/pbkdf2.rb +++ b/lib/devise/encryptable/encryptors/pbkdf2.rb @@ -5,18 +5,18 @@ module Encryptors class Pbkdf2 < Base def self.compare(encrypted_password, password, stretches, salt, pepper) value_to_test = self.digest(password, stretches, salt, pepper) - ActiveSupport::SecurityUtils.fixed_length_secure_compare(encrypted_password, value_to_test) + Devise.secure_compare(encrypted_password, value_to_test) end def self.digest(password, stretches, salt, pepper) - hash = OpenSSL::Digest::SHA512.new + hash = OpenSSL::Digest.new('SHA512').new OpenSSL::KDF.pbkdf2_hmac( - password, + password.to_s, salt: "#{[salt].pack('H*')}#{pepper}", iterations: stretches, hash: hash, length: hash.digest_length, - ).unpack('H*')[0] + ).unpack1('H*') end end end From 5b17777e62bf651821f543249b55e8a1e3347825 Mon Sep 17 00:00:00 2001 From: Tim Neems Date: Fri, 16 Sep 2022 13:47:32 -0400 Subject: [PATCH 5/5] Add a devise bcrypt encryptor for migrations to other hashing algorithms --- Changelog.md | 1 + lib/devise/encryptable/encryptable.rb | 1 + .../encryptable/encryptors/devise_bcrypt.rb | 33 ++++++++++++++++ test/devise/encryptable/encryptors_test.rb | 39 +++++++++++++++++++ 4 files changed, 74 insertions(+) create mode 100644 lib/devise/encryptable/encryptors/devise_bcrypt.rb diff --git a/Changelog.md b/Changelog.md index 3699e81..4159255 100644 --- a/Changelog.md +++ b/Changelog.md @@ -3,6 +3,7 @@ * Support Rails 4.1 - 7.0, dropped support for Rails <= 4.0. (same support as Devise as of v4.8) * Support Ruby 2.1 - 3.1, dropped support for Ruby <= 2.0. (same support as Devise as of v4.8) * Add support to roll legacy encryptors +* Add support for rolling from Devise BCrypt ### 0.2.0 diff --git a/lib/devise/encryptable/encryptable.rb b/lib/devise/encryptable/encryptable.rb index 71aeb05..441ed4e 100644 --- a/lib/devise/encryptable/encryptable.rb +++ b/lib/devise/encryptable/encryptable.rb @@ -22,6 +22,7 @@ module Encryptors autoload :AuthlogicSha512, 'devise/encryptable/encryptors/authlogic_sha512' autoload :Base, 'devise/encryptable/encryptors/base' autoload :ClearanceSha1, 'devise/encryptable/encryptors/clearance_sha1' + autoload :DeviseBcrypt, 'devise/encryptable/encryptors/devise_bcrypt' autoload :Pbkdf2, 'devise/encryptable/encryptors/pbkdf2' autoload :RestfulAuthenticationSha1, 'devise/encryptable/encryptors/restful_authentication_sha1' autoload :Sha1, 'devise/encryptable/encryptors/sha1' diff --git a/lib/devise/encryptable/encryptors/devise_bcrypt.rb b/lib/devise/encryptable/encryptors/devise_bcrypt.rb new file mode 100644 index 0000000..2c767af --- /dev/null +++ b/lib/devise/encryptable/encryptors/devise_bcrypt.rb @@ -0,0 +1,33 @@ +require 'bcrypt' + +begin + module Devise + module Encryptable + module Encryptors + # Adapted from + # https://github.com/heartcombo/devise/blob/8593801130f2df94a50863b5db535c272b00efe1/lib/devise/encryptor.rb + class DeviseBcrypt < Base + def self.compare(hashed_password, password, stretches, _salt, pepper) + return false if hashed_password.blank? + bcrypt = ::BCrypt::Password.new(hashed_password) + if pepper.present? + password = "#{password}#{pepper}" + end + password = ::BCrypt::Engine.hash_secret(password, bcrypt.salt) + Devise.secure_compare(password, hashed_password) + rescue BCrypt::Errors::InvalidHash + # this probably means the password has already been migrated + false + end + + def self.digest(password, stretches, _salt, pepper) + if pepper.present? + password = "#{password}#{pepper}" + end + ::BCrypt::Password.create(password, cost: stretches).to_s + end + end + end + end + end +end diff --git a/test/devise/encryptable/encryptors_test.rb b/test/devise/encryptable/encryptors_test.rb index 49a9323..35b6ced 100644 --- a/test/devise/encryptable/encryptors_test.rb +++ b/test/devise/encryptable/encryptors_test.rb @@ -1,4 +1,5 @@ require "test_helper" +require "ostruct" class Encryptors < ActiveSupport::TestCase include Support::Swappers @@ -21,6 +22,44 @@ class Encryptors < ActiveSupport::TestCase assert_equal clearance, encryptor end + test 'Devise can verify passwords generated by DeviseBcrypt' do + password = '123mudar' + klass = OpenStruct.new(pepper: '65c58472c207c829f28c68619d3e3aefed18ab3f', stretches: 10) + + hashed_password = Devise::Encryptable::Encryptors::DeviseBcrypt.digest(password, klass.stretches, nil, klass.pepper) + + assert Devise::Encryptor.compare(klass, hashed_password, password) + end + + test 'DeviseBcrypt can verify bcrypt hashes created by Devise' do + password = '123mudar' + klass = OpenStruct.new(pepper: '65c58472c207c829f28c68619d3e3aefed18ab3f', stretches: 10) + + devise_hash = Devise::Encryptor.digest(klass, password) + + assert Devise::Encryptable::Encryptors::DeviseBcrypt.compare(devise_hash, password, klass.stretches, nil, klass.pepper) + end + + test 'Devise fails to verify passwords generated by DeviseBcrypt when the password is wrong' do + password = '123mudar' + different_password = 'theskyisfalling123' + klass = OpenStruct.new(pepper: '65c58472c207c829f28c68619d3e3aefed18ab3f', stretches: 10) + + hashed_password = Devise::Encryptable::Encryptors::DeviseBcrypt.digest(password, klass.stretches, nil, klass.pepper) + + refute Devise::Encryptor.compare(klass, hashed_password, different_password) + end + + test 'DeviseBcrypt fails to verify bcrypt hashes created by Devise when the password is wrong' do + password = '123mudar' + different_password = 'theskyisfalling123' + klass = OpenStruct.new(pepper: '65c58472c207c829f28c68619d3e3aefed18ab3f', stretches: 10) + + devise_hash = Devise::Encryptor.digest(klass, password) + + refute Devise::Encryptable::Encryptors::DeviseBcrypt.compare(devise_hash, different_password, klass.stretches, nil, klass.pepper) + end + test 'digest should raise NotImplementedError if not implemented in subclass' do c = Class.new(Devise::Encryptable::Encryptors::Base) assert_raise(NotImplementedError) do