Skip to main content

This site requires you to update your browser. Your browsing experience maybe affected by not having the most up to date version.

Archive /

Our old forums are still available as a read-only archive.

Moderators: martimiz, Sean, biapar, Willr, Ingo, simon_w

A request for Markus...


Reply


6 Posts   1526 Views

Avatar
Sam

Administrator, 685 Posts

3 July 2007 at 9:53am

Hi Markus,

Could you add support for password encryption? :-)

I would suggest that you allow for putting something like this in your _config.php

Security::set_password_encryption('md5');
Security::set_password_encryption('password');

You'll notice that the two strings happen to both be mysql function names ;-)

To aid migration, adding an administrator tool like Security/encryptallpasswords, which would take plaintext passwords in the database and encrypt them according to the current password encrpytion. It would need some magic to prevent it from being called twice, though! :-)

Avatar
Markus

Google Summer of Code Hacker, 152 Posts

3 July 2007 at 8:30pm

Yes, of course. I can add support for password encryption, the easiest thing would be to add a column PasswordEncrypted or even better PasswordSalt (so we can use a salt) to the member table.
I would add that because Silverstripe doesn't support transactions and the process of encrypting all passwords can fail or exceed the time limit in the middle of the process.

I know that those two are MySQL function names, but I don't think that's a good idea to use the MySQL functions for that because it makes the database abstraction even more difficult. I think the easiest and most portable way is to just use the PHP built in functions.

But implementing this, we get another problem, namely the UI. At the moment in the security tab the passwords are shown in clear text and can be changed. If we add that new feature we have to change also the UI, but I don't know what the best way would be. Since the password entered there has to be encrypted, we have to add a "password change" feature and recovering the old one will be impossible after that change, so we need some other mechanism to use when someone forgot his password. The easiest way would be to generate a new one and mail it to the user which then afterwards can change it again.

Anyway, I can start implementing the Security::setPasswordEncryption(...) method and the Security/encryptallpasswords action if you give me the OK to do so...

Another problem: What should happen if someone changes the encryption method? Let’s say from MD5 to SHA1? All passwords are already encrypted according MD5, those can't be converted to SHA1, new ones will be encrypted automatically by SHA1.. I think that will get a big mess.. maybe we need to store also the encryption method if we use something like this.

I think it would be much easier to fix one encryption algorithm and just make a Security::encryptPasswords(bool). We can use any of the standard available algorithms.. maybe SHA!?

Tell me what you think!

Avatar
Sam

Administrator, 685 Posts

6 July 2007 at 1:45pm

I think that despite the issues that arise if someone changed the encryption method, it's worth having the ability to choose how you want them encrpyed. If you're integrating with another system it can be very helpful to use the same encryption method on your CMS as, for example, your phpBB forum.

That said, there should be a default option so that people who don't have a preference don't need to make an arbitrary decision.

It's going to be rare that someone wants to change the password encyption mid-stream, and if they do, it's acceptable that they have to mop the mess up themselves. We just need to document this clearly.

Having a separate Password and PasswordEncrypted fields leads to an unclear situation when you have both fields populated - which do you use? Another option is to have a PasswordEncryption enum field, which is none/password/md5/sha. This has the added benefit of dealing with changing encyption type mid-stream.

Avatar
Markus

Google Summer of Code Hacker, 152 Posts

6 July 2007 at 8:05pm

Edited: 06/07/2007 8:06pm

> I think that despite the issues that arise if someone changed the encryption
> method, it's worth having the ability to choose how you want them encrpyed.
> If you're integrating with another system it can be very helpful to use the
> same encryption method on your CMS as, for example, your phpBB forum.

That's a good point! But should we also use a salt? Otherwise the passwords are still very easy attackable with the help of rainbow tables.
I think with should add the option to use salts.

> That said, there should be a default option so that people who don't have
> a preference don't need to make an arbitrary decision.

OK

> It's going to be rare that someone wants to change the password encyption
> mid-stream, and if they do, it's acceptable that they have to mop the mess up
> themselves. We just need to document this clearly.

Sounds reasonable :-)

> Having a separate Password and PasswordEncrypted fields leads to an
> unclear situation when you have both fields populated - which do you use?

Maybe I didn't explain that enough.. I thought about a boolean: "Is the password encrypted?" -> bool PasswordEncrypted

> Another option is to have a PasswordEncryption enum field, which is
> none/password/md5/sha. This has the added benefit of dealing with
> changing encyption type mid-stream.

Hmm... but then the DB needs to know all available algorithms.. but anyway I think that's the best option.

So just to make sure I understand everything right and I'm going to implement the right things:

I will implement the following methods:
- encryptPasswords(bool)
- setPasswordEncryptionAlgorithm(string algo, bool use_salt)
(algo is one of the available algorithms determined by hash_algos())

The passwords will then be encrypted as follows

$password = (use_salt == true) ? hash($algorithm, $pwd . $salt) : hash($algorithm, $pwd);

where $salt is a randomly generated salt which is then stored in the Salt column in the member table (empty string or NULL if not used).

$algorithm will then also be stored in the member table in the column PasswordEncryption

I'll will on that after finishing the OpenID integration in the backend.
Please tell me if everything is OK in that way or if I should change some details.

Avatar
Sam

Administrator, 685 Posts

8 July 2007 at 9:28pm

It all looks good; the only comment I would have is use lowercase_with_underscores() format for the static functions.

Upon further investigation, I realised that http://doc.silverstripe.com/doku.php?id=coding-conventions wasn't up to date - sorry about the confusion here!

Avatar
Markus

Google Summer of Code Hacker, 152 Posts

9 July 2007 at 9:04pm

OK, fine. I created a [url=http://support.silverstripe.com/gsoc/ticket/34]ticket[/url] for that and will start to work on that soon.