diff options
| -rw-r--r-- | app/models/user.rb | 36 | ||||
| -rw-r--r-- | db/migrate/20110223180944_add_users_salt.rb | 9 | ||||
| -rw-r--r-- | db/migrate/20110223180953_salt_user_passwords.rb | 13 | ||||
| -rw-r--r-- | extra/svn/Redmine.pm | 9 | ||||
| -rw-r--r-- | test/fixtures/users.yml | 18 | ||||
| -rw-r--r-- | test/unit/user_test.rb | 34 |
6 files changed, 106 insertions, 13 deletions
diff --git a/app/models/user.rb b/app/models/user.rb index 5b0444016..025976831 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -83,7 +83,9 @@ class User < Principal def before_save # update hashed_password if password was set - self.hashed_password = User.hash_password(self.password) if self.password && self.auth_source_id.blank? + if self.password && self.auth_source_id.blank? + salt_password(password) + end end def reload(*args) @@ -121,7 +123,7 @@ class User < Principal return nil unless user.auth_source.authenticate(login, password) else # authentication with local password - return nil unless User.hash_password(password) == user.hashed_password + return nil unless user.check_password?(password) end else # user is not yet registered, try to authenticate with available sources @@ -200,13 +202,21 @@ class User < Principal update_attribute(:status, STATUS_LOCKED) end + # Returns true if +clear_password+ is the correct user's password, otherwise false def check_password?(clear_password) if auth_source_id.present? auth_source.authenticate(self.login, clear_password) else - User.hash_password(clear_password) == self.hashed_password + User.hash_password("#{salt}#{User.hash_password clear_password}") == hashed_password end end + + # Generates a random salt and computes hashed_password for +clear_password+ + # The hashed password is stored in the following form: SHA1(salt + SHA1(password)) + def salt_password(clear_password) + self.salt = User.generate_salt + self.hashed_password = User.hash_password("#{salt}#{User.hash_password clear_password}") + end # Does the backend storage allow this user to change their password? def change_password_allowed? @@ -473,6 +483,20 @@ class User < Principal end anonymous_user end + + # Salts all existing unsalted passwords + # It changes password storage scheme from SHA1(password) to SHA1(salt + SHA1(password)) + # This method is used in the SaltPasswords migration and is to be kept as is + def self.salt_unsalted_passwords! + transaction do + User.find_each(:conditions => "salt IS NULL OR salt = ''") do |user| + next if user.hashed_password.blank? + salt = User.generate_salt + hashed_password = User.hash_password("#{salt}#{user.hashed_password}") + User.update_all("salt = '#{salt}', hashed_password = '#{hashed_password}'", ["id = ?", user.id] ) + end + end + end protected @@ -514,6 +538,12 @@ class User < Principal def self.hash_password(clear_password) Digest::SHA1.hexdigest(clear_password || "") end + + # Returns a 128bits random salt as a hex string (32 chars long) + def self.generate_salt + ActiveSupport::SecureRandom.hex(16) + end + end class AnonymousUser < User diff --git a/db/migrate/20110223180944_add_users_salt.rb b/db/migrate/20110223180944_add_users_salt.rb new file mode 100644 index 000000000..f1cf6483f --- /dev/null +++ b/db/migrate/20110223180944_add_users_salt.rb @@ -0,0 +1,9 @@ +class AddUsersSalt < ActiveRecord::Migration + def self.up + add_column :users, :salt, :string, :limit => 64 + end + + def self.down + remove_column :users, :salt + end +end diff --git a/db/migrate/20110223180953_salt_user_passwords.rb b/db/migrate/20110223180953_salt_user_passwords.rb new file mode 100644 index 000000000..9f017db9c --- /dev/null +++ b/db/migrate/20110223180953_salt_user_passwords.rb @@ -0,0 +1,13 @@ +class SaltUserPasswords < ActiveRecord::Migration + + def self.up + say_with_time "Salting user passwords, this may take some time..." do + User.salt_unsalted_passwords! + end + end + + def self.down + # Unsalted passwords can not be restored + raise ActiveRecord::IrreversibleMigration, "Can't decypher salted passwords. This migration can not be rollback'ed." + end +end diff --git a/extra/svn/Redmine.pm b/extra/svn/Redmine.pm index c96b248c5..c0320f13e 100644 --- a/extra/svn/Redmine.pm +++ b/extra/svn/Redmine.pm @@ -148,7 +148,7 @@ sub RedmineDSN { my ($self, $parms, $arg) = @_; $self->{RedmineDSN} = $arg; my $query = "SELECT - hashed_password, auth_source_id, permissions + hashed_password, salt, auth_source_id, permissions FROM members, projects, users, roles, member_roles WHERE projects.id=members.project_id @@ -316,11 +316,12 @@ sub is_member { $sth->execute($redmine_user, $project_id); my $ret; - while (my ($hashed_password, $auth_source_id, $permissions) = $sth->fetchrow_array) { + while (my ($hashed_password, $salt, $auth_source_id, $permissions) = $sth->fetchrow_array) { unless ($auth_source_id) { - my $method = $r->method; - if ($hashed_password eq $pass_digest && ((defined $read_only_methods{$method} && $permissions =~ /:browse_repository/) || $permissions =~ /:commit_access/) ) { + my $method = $r->method; + my $salted_password = Digest::SHA1::sha1_hex($salt.$pass_digest); + if ($hashed_password eq $salted_password && ((defined $read_only_methods{$method} && $permissions =~ /:browse_repository/) || $permissions =~ /:commit_access/) ) { $ret = 1; last; } diff --git a/test/fixtures/users.yml b/test/fixtures/users.yml index f26c09c0b..1e612fc90 100644 --- a/test/fixtures/users.yml +++ b/test/fixtures/users.yml @@ -4,7 +4,9 @@ users_004: status: 1 last_login_on: language: en - hashed_password: 4e4aeb7baaf0706bd670263fef42dad15763b608 + # password = foo + salt: 3126f764c3c5ac61cbfc103f25f934cf + hashed_password: 9e4dd7eeb172c12a0691a6d9d3a269f7e9fe671b updated_on: 2006-07-19 19:34:07 +02:00 admin: false mail: rhill@somenet.foo @@ -20,7 +22,9 @@ users_001: status: 1 last_login_on: 2006-07-19 22:57:52 +02:00 language: en - hashed_password: d033e22ae348aeb5660fc2140aec35850c4da997 + # password = admin + salt: 82090c953c4a0000a7db253b0691a6b4 + hashed_password: b5b6ff9543bf1387374cdfa27a54c96d236a7150 updated_on: 2006-07-19 22:57:52 +02:00 admin: true mail: admin@somenet.foo @@ -36,7 +40,9 @@ users_002: status: 1 last_login_on: 2006-07-19 22:42:15 +02:00 language: en - hashed_password: a9a653d4151fa2c081ba1ffc2c2726f3b80b7d7d + # password = jsmith + salt: 67eb4732624d5a7753dcea7ce0bb7d7d + hashed_password: bfbe06043353a677d0215b26a5800d128d5413bc updated_on: 2006-07-19 22:42:15 +02:00 admin: false mail: jsmith@somenet.foo @@ -52,7 +58,9 @@ users_003: status: 1 last_login_on: language: en - hashed_password: 7feb7657aa7a7bf5aef3414a5084875f27192415 + # password = foo + salt: 7599f9963ec07b5a3b55b354407120c0 + hashed_password: 8f659c8d7c072f189374edacfa90d6abbc26d8ed updated_on: 2006-07-19 19:33:19 +02:00 admin: false mail: dlopper@somenet.foo @@ -70,7 +78,7 @@ users_005: status: 3 last_login_on: language: en - hashed_password: 7feb7657aa7a7bf5aef3414a5084875f27192415 + hashed_password: 1 updated_on: 2006-07-19 19:33:19 +02:00 admin: false mail: dlopper2@somenet.foo diff --git a/test/unit/user_test.rb b/test/unit/user_test.rb index 63e422701..3f324ddc4 100644 --- a/test/unit/user_test.rb +++ b/test/unit/user_test.rb @@ -361,7 +361,6 @@ class UserTest < ActiveSupport::TestCase user = User.try_to_login("admin", "hello") assert_kind_of User, user assert_equal "admin", user.login - assert_equal User.hash_password("hello"), user.hashed_password end def test_name_format @@ -383,6 +382,22 @@ class UserTest < ActiveSupport::TestCase assert_equal nil, user end + context ".try_to_login" do + context "with good credentials" do + should "return the user" do + user = User.try_to_login("admin", "admin") + assert_kind_of User, user + assert_equal "admin", user.login + end + end + + context "with wrong credentials" do + should "return nil" do + assert_nil User.try_to_login("admin", "foo") + end + end + end + if ldap_configured? context "#try_to_login using LDAP" do context "with failed connection to the LDAP server" do @@ -727,6 +742,23 @@ class UserTest < ActiveSupport::TestCase should 'be added and tested' end end + + def test_salt_unsalted_passwords + # Restore a user with an unsalted password + user = User.find(1) + user.salt = nil + user.hashed_password = User.hash_password("unsalted") + user.save! + + User.salt_unsalted_passwords! + + user.reload + # Salt added + assert !user.salt.blank? + # Password still valid + assert user.check_password?("unsalted") + assert_equal user, User.try_to_login(user.login, "unsalted") + end if Object.const_defined?(:OpenID) |