-
-
Notifications
You must be signed in to change notification settings - Fork 891
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
policies: add unique password policy #10631
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for authentik-storybook ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for authentik-docs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
90a6866
to
8a89b13
Compare
for history in password_history: | ||
old_password = history.change.get("old_password") | ||
if not old_password: | ||
# TODO: how do we handle missing password? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feedback requested: how should the policy behave if the UserPasswordHistory
record doesn't have an old_password
key?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we indeed go with a field on the object, we avoid having to ask ourselves those questions.
LOGGER.warning( | ||
"Could not load hash algorithm for old password.", | ||
) | ||
# TODO: Define behavior if hasher cannot be identified or is unsupported |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the policy fail if the Password hash algorithm was removed from Django? I know at least 2 are scheduled for deprecation in 5.1.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should just ignore unknown algos, and maybe schedule a task to remove ones we don't know about?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ignoring them works for me. Django won't be able to compare against that password, and in this implementation it might as well not exist.
- Ignore passwords where we can't identify the algo
maybe schedule a task to remove ones we don't know about
I'm not sure the overhead of running a task (and spending the time to write it) would get us much in return. Passwords are already cleared out in a LIFO-esque way, so eventually the unknown password would age out.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #10631 +/- ##
===========================================
- Coverage 92.59% 50.84% -41.75%
===========================================
Files 720 718 -2
Lines 35254 35196 -58
===========================================
- Hits 32642 17896 -14746
- Misses 2612 17300 +14688
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some design comments, most of which will need to be checked by @BeryJu anyway
No specific review on the frontend, cause I wouldn't know what I'm saying.
I think we should also provide a way for administrators to clear the password history of a specific user (and all users?) so that a user can be unblocked from changing their password if needed.
All in all, a pretty solid implementation
for history in password_history: | ||
old_password = history.change.get("old_password") | ||
if not old_password: | ||
# TODO: how do we handle missing password? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we indeed go with a field on the object, we avoid having to ask ourselves those questions.
fields=request.context.keys(), | ||
) | ||
return PolicyResult(False, _("Password not set in context")) | ||
password = str(password) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should already be the case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be 🤷 The other Password policy applies the same str
casting and I figured that was done on purpose.
I can remove if @BeryJu feels the same way
@rissson I've applied the suggested changes. Namely:
❗ One issue I'm stumped on:
Resolved after fixing #10631 (comment) [1] Name of the model manager method is definitely up for discussion |
Also I'll rebase and get the schema conflict resolved before merging |
"""QuerySet for filtering enabled bindings for a Policy type""" | ||
|
||
def for_policy(self, policy: "Policy"): | ||
return self.filter(policy__in=policy._default_manager.all()).filter(enabled=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to make sure I understand, why are we referring to the default manager here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I chose _default_manager
because I wanted to preserve the behavior of the InheritanceManager()
managers on the different Policy
subclasses. Is that unnecessary here?
…ry table If the UniquePasswordPolicy is enabled anywhere, we now record the user's hashed password.
…bound to anything
The system should aim to keep the number of historical passwords to a minimum to avoid wasting storage space. Admins can configure how many passwords they want to preserve. If multiple instances of the UniquePasswordPolicy exist, the system takes the max() value of all enabled policies to determine how many passwords should remain after trimming.
…ings Querying within post_delete for other PolicyBindings will include the PolicyBinding we're deleting. We have to account for that by checking for bindings other than the one we're looking at.
…nc task instead of prior to execution
… to a provided policy type
…PasswordPolicyForm
…rom ManagedAppConfig.
e98fc6a
to
4ff7b9b
Compare
Added public documentation in a separate PR: #11000 |
Details
Closes #8307
Adds a "Password Uniqueness" policy to the set of configurable flow policies.
Documentation PR: #11000
How it works
After at least one (1) Flow has a UniquePasswordPolicy binding attached & enabled, the system records every user's hashed password when the user changes their own password.
When the user submits a new password during a password-change Flow, the system will check if the new password is identical to any of the user's previous passwords. The check will hash the new password against the Hasher originally used to hash the old password. This ensures backwards compatiblity if Authentik decides to change hashing algorithsm.
This check only occurs if the Flow Stage has a Unique Password Policy attached.
Configuration
Admins can configure how many previous passwords the system should evaluate.
The configured number of passwords to evaluate also defines the number of passwords retained for each user.
Admins may also define which field they wish to use as the "password" field in a Stage. This is similar to the Password Policy configuration.
Data considerations
The system purges the password history table after the last enabled UniquePasswordPolicy binding is deleted.
Because the number of passwords we save is configurable, it's important admins understand that whatever value they configure will end up saving up to
n^m
passwords, weren
is the configured policy value &m
is the number of users.Checklist
ak test authentik/
)make lint-fix
)If an API change has been made
make gen-build
)If changes to the frontend have been made
make web
)If applicable
make website
)