Ini auth + support for sha1 hashes

[Steve Clay]

Hello,
I've just started experimenting with Solar. I'm admittedly a bit green
to the full-featured MVC frameworks, so bear with me.

I noticed Solar_Auth_Adapter_Ini had no support for hashed passwords, so
I added it (with usage in the class doc):
http://paste2.org/p/424459

I'll send in the CLA tomorrow (whether or not y'all want this particular
change).

Cheers
--
Steve Clay
http://mrclay.org/
_______________________________________________
Solar-talk mailing list
Solar-talk@...
http://mailman-mail5.webfaction.com/listinfo/solar-talk

[Paul M Jones-4]

On Sep 13, 2009, at 20:50 , Steve Clay wrote:

> I've just started experimenting with Solar. I'm admittedly a bit green
> to the full-featured MVC frameworks, so bear with me.

Hi Mr Clay, nice to see you.  :-)


> I noticed Solar_Auth_Adapter_Ini had no support for hashed  
> passwords, so
> I added it (with usage in the class doc):
> http://paste2.org/p/424459

Oooo, that's interesting.  I never thought to do anything like that.

I wonder: instead of a separate 'sha1' key/value pair, would it be  
useful to do something like Apache Htpasswd, and prefix the password  
string with the hash type?  E.g., "passwd=sha1:jdklkjsadidf". Then you  
could split on the colon, and use the first part as the first argument  
to the hash() function. That would allow for md5 and possibly other  
hashing algo.


> I'll send in the CLA tomorrow (whether or not y'all want this  
> particular change).

Looking forward to receiving it.  Welcome aboard.


--

Paul M. Jones
http://paul-m-jones.com/




_______________________________________________
Solar-talk mailing list
Solar-talk@...
http://mailman-mail5.webfaction.com/listinfo/solar-talk

[Paul M Jones-4]

On Sep 13, 2009, at 21:13 , Paul M Jones wrote:

> On Sep 13, 2009, at 20:50 , Steve Clay wrote:
>
>> I've just started experimenting with Solar. I'm admittedly a bit  
>> green
>> to the full-featured MVC frameworks, so bear with me.
>
> Hi Mr Clay, nice to see you.  :-)
>
>
>> I noticed Solar_Auth_Adapter_Ini had no support for hashed
>> passwords, so
>> I added it (with usage in the class doc):
>> http://paste2.org/p/424459
>
> Oooo, that's interesting.  I never thought to do anything like that.
>
> I wonder: instead of a separate 'sha1' key/value pair, would it be
> useful to do something like Apache Htpasswd, and prefix the password
> string with the hash type?  E.g., "passwd=sha1:jdklkjsadidf". Then you
> could split on the colon, and use the first part as the first argument
> to the hash() function. That would allow for md5 and possibly other
> hashing algo.

Hm, or maybe a key/value pair like 'passwd_hash = sha1' to indicate  
the hashing algo.  That'd probably be a better approach than "passwd =  
sha1:askasldfjk".




--

Paul M. Jones
http://paul-m-jones.com/




_______________________________________________
Solar-talk mailing list
Solar-talk@...
http://mailman-mail5.webfaction.com/listinfo/solar-talk

[Steve Clay]

Paul M Jones wrote:
>> could split on the colon, and use the first part as the first argument
>> to the hash() function. That would allow for md5 and possibly other
>> hashing algo.
>
> Hm, or maybe a key/value pair like 'passwd_hash = sha1' to indicate  
> the hashing algo.  That'd probably be a better approach than "passwd =  
> sha1:askasldfjk".

Here's another try: http://paste2.org/p/425466

[pmjones]
hash = 88f32bccbc8cd02d56f1048dd0...
hash_salt = "r74y38%&hu4u&^%%y9=^6^^45#EjiU(90fg67"
hash_algo = whirlpool

I thought it best to leave out 'passwd', because... it isn't one? And if
you use hash, a salt is required (otherwise the whole point is defeated).

I also removed all the auth config keys from $user before returning it.
It seems reckless to keep around $user['passwd'] when it's no longer
needed, especially if $user gets stuck in a session (I don't know if
Solar does this or not).

So security has risen marginally and usability dropped significantly :)

--
Steve Clay
http://mrclay.org/
_______________________________________________
Solar-talk mailing list
Solar-talk@...
http://mailman-mail5.webfaction.com/listinfo/solar-talk

[Paul M Jones-4]

Hey Steve,

Let me work backwards here.

> So security has risen marginally and usability dropped  
> significantly :)

Heh.  :-)

>>> could split on the colon, and use the first part as the first  
>>> argument
>>> to the hash() function. That would allow for md5 and possibly other
>>> hashing algo.
>>
>> Hm, or maybe a key/value pair like 'passwd_hash = sha1' to indicate
>> the hashing algo.  That'd probably be a better approach than  
>> "passwd =
>> sha1:askasldfjk".
>
> Here's another try: http://paste2.org/p/425466
>
> [pmjones]
> hash = 88f32bccbc8cd02d56f1048dd0...
> hash_salt = "r74y38%&hu4u&^%%y9=^6^^45#EjiU(90fg67"
> hash_algo = whirlpool
>
> I thought it best to leave out 'passwd', because... it isn't one?  
> And if
> you use hash, a salt is required (otherwise the whole point is  
> defeated).

Well, it's the stored representation of the password.  I like the idea  
of having all the related pieces named similarly.  E.g.:

[pmjones]
passwd = 88f32bccbc8cd02d56f1048dd0...
passwd_salt = r74y38%&hu4u&^%%y9=^6^^45#EjiU(90fg67
passwd_hash = whirlpool

Also, instead of returning false if the algo isn't found, I'd say  
throw an exception.

throw $this->_exception('ERR_NO_SUCH_ALGO', array(
     'passwd_hash' => $user['passwd_hash'],
     'available'   => hash_algos(),
));

Aside from that, I think the patch looks reasonable.



> I also removed all the auth config keys from $user before returning  
> it.
> It seems reckless to keep around $user['passwd'] when it's no longer
> needed, especially if $user gets stuck in a session (I don't know if
> Solar does this or not).

Hm, that didn't occur to me.  I wonder if that needs to be done for  
all of the adapters.




--

Paul M. Jones
http://paul-m-jones.com/




_______________________________________________
Solar-talk mailing list
Solar-talk@...
http://mailman-mail5.webfaction.com/listinfo/solar-talk

[Paul M Jones-4]

On Sep 14, 2009, at 22:37 , Paul M Jones wrote:

> Aside from that, I think the patch looks reasonable.

Oh sorry I had two more questions:

1. If passwd_salt is not present, is that OK?

2. Do we want passwd_salt to be per-user, or do we want to define one  
via the config file to be used by everyone?


--

Paul M. Jones
http://paul-m-jones.com/




_______________________________________________
Solar-talk mailing list
Solar-talk@...
http://mailman-mail5.webfaction.com/listinfo/solar-talk

[Steve Clay]

Paul M Jones wrote:
> 1. If passwd_salt is not present, is that OK?
>
> 2. Do we want passwd_salt to be per-user, or do we want to define one  
> via the config file to be used by everyone?

It depends on the goal of this feature. If the goal is obscuring
passwords from casual viewers of the .ini, any hash is acceptable (no
salt needed).

If the goal is to legitimately secure passwords from cracking, we need
to guarantee unique, random salts and an expensive hash function that
can be made more expensive as computers get faster. The industry
standard is mcrypt (in OpenBSD) and thankfully there's a well-tested PHP
implementation: http://www.openwall.com/phpass/

It would be good to bring this hashing everywhere passwords are stored.

My patches were better than nothing, but only marginally.

--
Steve Clay
http://mrclay.org/
_______________________________________________
Solar-talk mailing list
Solar-talk@...
http://mailman-mail5.webfaction.com/listinfo/solar-talk

[Paul M Jones-4]

On Sep 15, 2009, at 16:08 , Steve Clay wrote:

> Paul M Jones wrote:
>> 1. If passwd_salt is not present, is that OK?
>>
>> 2. Do we want passwd_salt to be per-user, or do we want to define one
>> via the config file to be used by everyone?
>
> It depends on the goal of this feature. If the goal is obscuring
> passwords from casual viewers of the .ini, any hash is acceptable (no
> salt needed).
>
> If the goal is to legitimately secure passwords from cracking, we need
> to guarantee unique, random salts and an expensive hash function that
> can be made more expensive as computers get faster.

A fair and forward-looking answer.  I think we can allow for both  
config-based and per-user salts, then.  If a user salt is available,  
use it; fall back to a config salt for all; fall back to no salt.  
Would that be an acceptable course?


> The industry standard is mcrypt (in OpenBSD) and thankfully there's  
> a well-tested PHP
> implementation: http://www.openwall.com/phpass/

Well now *that's* interesting.  Wonder if that means there's room for  
an mcrypt-extension-based auth adapter, too.


> It would be good to bring this hashing everywhere passwords are  
> stored.

Well, it can't be *everywhere* -- the LDAP auth adapter, for example.  
But the SQL adapter might be able to use the same kind of fallback  
mechanism: when a salt_col is present, use it; fall back to a config  
salt for all; fall back to no salt.


> My patches were better than nothing, but only marginally.

I'll take incremental improvements over none at all when the cost-
benefit ratio is acceptable, and I think it definitely is in this case.




--

Paul M. Jones
http://paul-m-jones.com/




_______________________________________________
Solar-talk mailing list
Solar-talk@...
http://mailman-mail5.webfaction.com/listinfo/solar-talk