Open r2evans opened 8 years ago
Interesting points. I'd have to agree @michag86 re: failure delays, I think it's probably outside the scope of this app.
As for password expiration, this is a requirement of our internal security policy at my work. Currently we manage it manually (for lots of services), and I can see the advantage of having it built into the app. I'm not sure I like the idea of storing password historical password hashes, I think I would go with a timestamp for tracking when the password was last changed.
@nickbooties, I certainly understand your concern about preventing password reuse. I agree with many of the arguments that some of these perhaps-draconian security measures effectively decrease security, though I cannot back it up with anything empirical. Lacking solid proof to either side, would you agree that this kind of decision belongs in the hands of a well-informed administrator? (I'm willing to admit that, though I believe the policy may be flawed, there may be utility in it as well.)
If I understand things correctly, then the salt (and pepper?) changes with each password hashing; this could cause a problem, but it should be possible to force the salt in password_hash
.
@michag86, I think you two are right that failure delays and lockout are perhaps more appropriate in a different app. I'm on hostmonster so I believe I cannot install fail2ban, so I'll look into other options. Though they all fit under the auspices of "Security", perhaps having a different app for attack/intrusion detection is better.
NB: I'm gathering some support for this from here.
OK, after a little bit more code-searching, it appears that core OC does not in fact use password_hash
... it's using a class that implements an allegedly-future-proof hasher that utilizes a fixed salt that is stored in config/config.php
. So, it's slightly less complicated.
The thoughts from here on out are incomplete.
Things that I propose may work (I'm working on code now):
./admin.php
and js/admin.js
: two new variables: expiration
(integer, 0 disables) and pwhistory
(integer, 0 disables). Both default to "0" so that nobody is "surprised".templates/{admin,user}.php
updated to set and inform, respectively.appinfo/database.xml
; I think we need the following fields: id
, user_id
, pw_hash
, and date_set
. If the decision is made to not track password history, then the third field can be ignored or excluded. Question: what should the varchar lengths be for user_id
and pw_hash
? I'm defaulting to 200 at the moment, may be overkill for the hash which is currently 62 bytes, but I don't know if we need to accept futurre-different hashing.preSetPassword(...)
: actually, updating validatepassword
will suffice, just need to add a DB query to check for a potential history hit.postSetPassword(...)
: to add the new row to the database. I think it's probably better doing it here vice in validatepassword
. This is also where we need to delete extra hashes.Even if the pwhistory
variable (as I'm calling it) is set to 0 (disable), we need to create at least one row -- even if the hash is not stored -- so that we can track date_set
to look for expiration. In fact, it makes sense to store an empty string for the hash if pwhistory
is 0, if for no other reason than some concerned admins may disable this feature in order to NOT store a hash, so though a row will be created, it'll be used just for the date_set
.
I haven't figured out yet how to implement password expiration. Need to integrate into the user authentication somehow.
Some questions:
I don't think that we need a separate table (appinfo/database.xml) for storing the password history. Api provides setUserValue (http://api.owncloud.org/classes/OCP.Config.html#setUserValue) which can be used for storing the password hashs.
For Password expiration, the hook preLogin can be used. I think an implementation like https://github.com/jmeile/agreedisclaimer/ would be nice. If the user enters his password and the account is expired, he gets the login page again with a new input field "new password".
Oh, great, I was not familiar with the {get,set}UserValue
functions, I'll shift efforts, that greatly simplifies some steps. It isn't a perfect match, since with a history we'll need to store the nth
previous password within the key, e.g. key=prev1, value=
I stopped just short of reading the preLogin
docs, that makes perfect sense. I forked, so I'll try to provide a look into my offered code later.
I know it's been mentioned on apps.owncloud.org, but not surprisingly that isn't the best place to discuss it. First thing, however, I really like the clean/simple configuration and functionality of
passwordpolicy
. Thanks for contributing it!What are your thoughts on adding these features?
Thank you again for the app, it does a great job filling a niche-requirement.