Blowfish Clarifications. #948

Merged
merged 1 commit into from Nov 13, 2012

Conversation

Projects
None yet
3 participants
Contributor

sitedyno commented Nov 11, 2012

Attempt to make blowfish usage clearer/easier as requested here. I'll be happy to change or cherry pick out things as needed :)

@ADmad ADmad commented on the diff Nov 11, 2012

lib/Cake/Utility/Security.php
@@ -222,7 +235,7 @@ public static function rijndael($text, $key, $operation) {
* @param integer $length The length of the returned salt
* @return string The generated salt
*/
- public static function salt($length = 22) {
+ protected static function _salt($length = 22) {
@ADmad

ADmad Nov 11, 2012

Member

Why change it to protected?

@sitedyno

sitedyno Nov 11, 2012

Contributor

It's not directly tested and it being protected implies the end developer doesn't need to/shouldn't use it. In other words: you don't need to bother with salts.

@markstory

markstory Nov 12, 2012

Owner

I'm fine with it moving to protected, @sitedyno makes some good points. Also 2.3 hasn't gone stable so this isn't a big break in compatibility.

@ADmad

ADmad Nov 12, 2012

Member

While the salt string generated is used only for hashing internally it could be useful as a random string generator which is often needed for various purposed in an app. Having this function as public would avoid having to write your own function.

Just a suggestion though. I am fine with making it protected if that's the general consensus.

@markstory

markstory Nov 12, 2012

Owner

Well we could always add a random() method to String class if that is something people will frequently need. I don't think it should belong on Security unless we're going to get crpyto strength random data which is harder to come by on most hosts.

@ADmad

ADmad Nov 12, 2012

Member

Fair enough, so I guess this is ready to merge once the commits are squashed.

1.1 had a NeatString::randomPassword() function. We could reinstate that as String::random() or have a better random string generator logic.

@markstory markstory commented on an outdated diff Nov 12, 2012

lib/Cake/Utility/Security.php
- * Create a hash from string using given method.
- * Fallback on next available method.
- * If you are using blowfish, for comparisons simply pass the originally hashed
- * string as the salt (the salt is prepended to the hash and php handles the
- * parsing automagically. Do NOT use a constant salt for blowfish.
+ * Create a hash from string using given method or fallback on next available method.
+ *
+ * #### Using Blowfish
+ *
+ * -Creating Hashes: *Do not supply a salt*. Cake handles salt creation for
+ * you ensuring that each hashed password will have a *unique* salt.
+ * -Comparing Hashes: Simply pass the originally hashed password as the salt.
+ * The salt is prepended to the hash and php handles the parsing automagically.
+ * For convenience the BlowfishAuthenticate adapter is available for use with
+ * the AuthComponent.
+ * -Do NOT use a constant salt for blowfish!
@markstory

markstory Nov 12, 2012

Owner

You should have spaces after these - or the api docs won't understand what you mean.

@markstory markstory commented on an outdated diff Nov 12, 2012

lib/Cake/Utility/Security.php
+ * #### Using Blowfish
+ *
+ * -Creating Hashes: *Do not supply a salt*. Cake handles salt creation for
+ * you ensuring that each hashed password will have a *unique* salt.
+ * -Comparing Hashes: Simply pass the originally hashed password as the salt.
+ * The salt is prepended to the hash and php handles the parsing automagically.
+ * For convenience the BlowfishAuthenticate adapter is available for use with
+ * the AuthComponent.
+ * -Do NOT use a constant salt for blowfish!
+ *
+ * Creating a blowfish/bcrypt hash:
+ *
+ * {{{
+ * Security::setHash('blowfish');
+ * $hash = Security::hash('secretpassword');
+ * }}}
@markstory

markstory Nov 12, 2012

Owner

I wouldn't recommend using setHash() instead why not recommend $hash = Security::hash($password, 'blowfish');

Contributor

sitedyno commented Nov 12, 2012

Last commit fixed the formatting too. I'll gladly squash up commits once you guys are happy.

Owner

markstory commented Nov 13, 2012

@sitedyno I think this looks good.

Contributor

sitedyno commented Nov 13, 2012

@markstory It's been squashed up ;)

@markstory markstory added a commit that referenced this pull request Nov 13, 2012

@markstory markstory Merge pull request #948 from sitedyno/blowfish-updates
Blowfish Clarifications.
eb3e509

@markstory markstory merged commit eb3e509 into cakephp:2.3 Nov 13, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment