search

biviosoftware / perl-Bivio

Bivio OSS Platform
0 stars 1 forks source link

Fix #67 Validate that new passwords aren't weak with various criteria #72

Closed schellj closed 1 year ago

schellj commented 1 year ago

Builds upon #70, #71.

Checks that new passwords do not match user id, login name (i.e., realm owner name), or email address. Also checks that new passwords don't appear in a configurable weak password corpus. Default configuration is to use a corpus that is a simple, newline delineated text file containing a list of disallowed (i.e., common, easily guessable, or breached) passwords that is not overly large, such as https://github.com/dropbox/zxcvbn/blob/master/data/passwords.txt. A larger corpus would probably necessitate the in_weak_corpus implementation referring to an external database.

schellj commented 1 year ago

Considering this along with #71, it makes more sense to have a type PasswordV2, which would be used for new passwords and confirmations. Sorry for jumping to conclusions. Using different types is better than adding new methods, because it reuses common infrastructure.

I'm not sure what you mean by jumping to conclusions.

robnagler commented 1 year ago

I'm not sure what you mean by jumping to conclusions.

I said to have PasswordLegacy (for login password), but I think PasswordV2 (for new and confirmed) is better. I'm actually a bit on the fence as it is generally problematic so your solution might be fine.

schellj commented 1 year ago

I said to have PasswordLegacy (for login password), but I think PasswordV2 (for new and confirmed) is better. I'm actually a bit on the fence as it is generally problematic so your solution might be fine.

Any more thoughts on this? Is it worth trying to factor the changes in #71, #72, #73 into a solution with a new password type?

robnagler commented 1 year ago

I think using two password types (existing and new) is appropriate. They are quite different behaviors. Also, ConfirmPassword doesn't need any checks so maybe that's a third type (may be already).

schellj commented 1 year ago

I think using two password types (existing and new) is appropriate. They are quite different behaviors. Also, ConfirmPassword doesn't need any checks so maybe that's a third type (may be already).

It seems that there should also be a password hash type (plus sub-types, as seen in #73), i.e., what's stored in RealmOwner.password, as well as a type used for old/existing password form fields, a type used for new password fields, and a type used for confirm new password fields. So, really 4+ types.

It's strange that right now Type.Password is used both for the type of RealmOwner.password (i.e., the hashed clear text), and the password form field types (i.e., the clear text), and, as such, the Type.Password min and max widths get applied to both types of data.

robnagler commented 1 year ago

The way it works is that the "hash type" is not distinct in that it is a representation of the same value in a different form (clear text vs what's stored in the db).

The new vs existing is a bit of a hack as the behavior is really attached to the form/model, not the value of the type itself. It could be outside the type, but I think this code works ok as is.

I agree that "min" and "max" are not exactly right, because it should depend on the representation of the value. This is also true of Type.Enum, where min/max is about the UI representation not the db representation. It works, because we don't generate the DDL from the Type. If we did, we'd have to introduce other functions for representing the type on SQL.

schellj commented 1 year ago

Obviated by #74.