Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implements password validation via Django #1658

Merged
merged 4 commits into from Sep 25, 2018
Merged

Implements password validation via Django #1658

merged 4 commits into from Sep 25, 2018

Conversation

strikaco
Copy link
Contributor

Brief overview of PR changes/additions

Evennia's current password validation functionality is copied and pasted across many Command objects and given the contextual impositions of Commands, are completely unavailable from the web server. This leads to unsafe and hard-coded policies such a default minimum password length of 4 characters and allowing public-known compromised passwords to be used.

This PR enables the 4 default validators that would otherwise be enabled by default in any new Django 1.9+ project, sets the default minimum password length to a more sane 8 (can be reverted in settings.py/server.conf per Django conventions) and adds a 5th validator based on the character set Evennia currently allows.

It does not attempt to integrate with or modify any of the default commands or impact users' existing passwords, but any stock Django forms (including the Admin interface) will enforce these policies on new passwords.

Motivation for adding to Evennia

Adherence to DRY principles and more consistent application of password policy going forward.

Copy link
Member

@Griatch Griatch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good overall, but a few points:

  • The contrib validator has no instructions for how to actually use it in Evennia code. I know this can be looked up in Django docs, but that's not good enough for a contrib.
  • Does the minimum password length affect existing password validation? If so this appears inconsistent - would be bad if you can log into the game as user #1 but not into the admin with the same account and password. I can see the point in increasing the minimum (although everything under 11 characters won't stand up well to cracking anyway I guess), but maybe it could only happen for newly created games.
  • Inconsistency between game and web is no good. There are not that many places where the password is checked in regular game code. It sounds like a good time to integrate this in those places?
  • If the previous point is done and the Evennia validator is used by default, I guess it doesn't belong in the contrib/ folder but should be moved into the evennia/server/ folder.

@strikaco
Copy link
Contributor Author

strikaco commented Sep 20, 2018

The contrib validator has no instructions for how to actually use it in Evennia code. I know this can be looked up in Django docs, but that's not good enough for a contrib.

The validator should not be getting called directly though; it undermines the purpose of using the Django validation framework. If you're manually calling the contrib validator, that means you're bypassing the others. This is not a good practice to get in the habit of.

I think the actual problem here is the lack of a common API call to set/change passwords that forces validation, like however the Admin interface and stock password change forms do it. That would be worth implementing and documenting; more on this below. Given the status quo, if a dev rolls their own password-change functionality (and there are many valid reasons), it's quite trivial for them to inadvertently bypass all validation by calling low-level methods on high-level objects without understanding the implications.

>>> from django.conf import settings

# Passwords should be secure!
>>> settings.AUTH_PASSWORD_VALIDATORS
[{'NAME': 'django.contrib.auth.password_validation.UserAttributeSimilarityValidator'}, {'NAME': 'django.contrib.auth.password_validation.MinimumLengthValidator', 'OPTIONS': {'min_length': 8}}, {'NAME': 'django.contrib.auth.password_validation.CommonPasswordValidator'}, {'NAME': 'django.contrib.auth.password_validation.NumericPasswordValidator'}, {'NAME': 'evennia.contrib.security.validators.EvenniaPasswordValidator'}]

# Get an Account and set a password
>>> from typeclasses.accounts import Account
>>> Account.objects.first()
<Account: johnny(account#1)>
>>> a = Account.objects.first()

# ...but this lets me set a weak password...
>>> a.set_password('1234')

# ...oops, I did it again.
>>> a.set_password('#')

# Neither Evennia nor Django policy should have allowed this password
>>> a.check_password('#')
True
# ...but it did.

Does the minimum password length affect existing password validation? If so this appears inconsistent - would be bad if you can log into the game as user #1 but not into the admin with the same account and password. I can see the point in increasing the minimum (although everything under 11 characters won't stand up well to cracking anyway I guess), but maybe it could only happen for newly created games.

Nope! I agree with your concern, but the low impact I'm estimating is the only reason I felt comfortable pushing this in the first place. If your existing password is something trivial like "1234" you will be allowed to continue to use it in perpetuity whether it's on the web or in a terminal. However with these validators enabled, when the time comes for you to change it/set it to something else (or create a new account), the stock web interface will force adherence to the new requirements. The existing terminal commands (as currently implemented) will not.

Inconsistency between game and web is no good. There are not that many places where the password is checked in regular game code. It sounds like a good time to integrate this in those places?

CmdPassword, CmdNewPassword, and CmdUnconnectedCreate are three commands that hard-code the checks and should be corrected to have that block removed (I also need to modify the connection screen to reflect the stated policy). There are a bunch of interrelated helper functions that handle things like account creation too though, which is the more daunting rabbithole.

Would it be an overreach to override the Account.set_password() method (and maybe create an Account.validate_password() method to accomplish point 1) to simply use the framework as intended? This would force consistency regardless of calling function. Then it would be transparent to everyone with no special implementation details, and entirely configurable via server.conf.

The tradeoff is that a dev would no longer be able to override the password policy and manually set a password of poor complexity, but in my professional experience no good ever comes of that (and if they want to be able to set crappy passwords, they can disable the validators and deal with the consequences). Or I can set a flag to allow skipping of the validation check.

@Griatch
Copy link
Member

Griatch commented Sep 20, 2018

@strikaco

  • Cool, if it doesn't collide with existing passwords it's indeed not a hurdle to add this at all :)
  • I think it sounds like a good idea to implement something along the lines of Account.set_password/validate_password making use of as much of Django's resources as possible. These are things that makes little sense for most devs to mess with - these are the 'boring bits' people expect Evennia to handle for them. If we can say "just use this - it's probably safer than anything you can come up with", it's only for the best IMO. If they really want to customize things they can always use their own Account typeclass anyway. :)

@strikaco
Copy link
Contributor Author

@strikaco

* Cool, if it doesn't collide with existing passwords it's indeed not a hurdle to add this at all :)

* I think it sounds like a good idea to implement something along the lines of `Account.set_password/validate_password` making use of as much of Django's resources as possible. These are things that makes little sense for most devs to mess with - these are the 'boring bits' people expect Evennia to handle for them. If we can say "just use this - it's probably safer than anything you can come up with", it's only for the best IMO. If they _really_ want to customize things they can always use their own Account typeclass anyway. :)

Awesome, we're on the same page. I think I'll add a few hooks while I'm at it to make things like #1383 more of a possibility; password changes are a sensitive-enough event worth having the ability to yield notifications before or after the fact.

…alidate_password() method to validate passwords.
…Django password validation before modification.
@strikaco
Copy link
Contributor Author

@Griatch Ready for review.

  • DefaultAccount now has an additional function, validate_password(), and an extension of set_password().
  • The three default password-handling commands have been modified to validate against the Account object instead of implementing their own checks.
  • set_password will always validate the password before committing, regardless of the degree of abstraction the request came from (or whether it came from an account creation request or a password change request), and I did leave in a flag to ignore validation upon request.
  • There is now an Account.at_password_change() hook that runs after a successful password change.
  • Advancing the cause of Feature Request: More security-related info sent to console/logged by the network portal #1383, a generic statement concerning the fact that a password change was made to an account object is now logged to the standard Evennia logger.

@Griatch Griatch added this to To do in Evennia 0.8 Sep 22, 2018
@Griatch Griatch merged commit ef6be02 into evennia:develop Sep 25, 2018
Evennia 0.8 automation moved this from To do to Done Sep 25, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Evennia 0.8
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants