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

policies/password: password uniqueness history #13453

Open
wants to merge 38 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
38 commits
Select commit Hold shift + click to select a range
eb56da4
Add UniquePasswordPolicy model and migration
verkaufer Jul 23, 2024
798af64
Add UserPasswordHistory model and migration.
verkaufer Jul 23, 2024
f57d7fb
Add serializer for UniquePasswordPolicy
verkaufer Jul 24, 2024
7ee7115
Add UniquePasswordPolicy config to schema
verkaufer Jul 24, 2024
771ef97
Implement UniquePasswordPolicy algorithm
verkaufer Jul 23, 2024
8de2221
fix: Return error message when UniquePasswordPolicy fails
verkaufer Jul 27, 2024
f067fd9
on_user_write signal now records user's password to UserPasswordHisto…
verkaufer Jul 27, 2024
ff79ad6
Introduce PasswordPolicyUniquenessForm to PolicyWizard
verkaufer Jul 27, 2024
909aaaa
Update UniquePasswordPolicy component property
verkaufer Jul 27, 2024
94f7d69
Delete all UserPasswordHistory records when UniquePasswordPolicy not …
verkaufer Jul 27, 2024
d678c0c
Trim the UserPasswordHistory table when user updates their own password.
verkaufer Jul 27, 2024
be3b144
fix: Exit purge_password_history if there are 2 or more existing bind…
verkaufer Jul 31, 2024
e942c3e
Add comment to root policies/app.py explaining app structure
verkaufer Aug 2, 2024
ee06b8a
Move UniquePasswordPolicy to its own policies sub-app
verkaufer Aug 2, 2024
8c93822
Move unique password policy tasks and signals into policies.unique_pa…
verkaufer Aug 4, 2024
4d812c0
Move signal handler for password history updates to unique_password app
verkaufer Aug 4, 2024
0a6bcef
purge_password_history_table now checks for other bindings during asy…
verkaufer Aug 4, 2024
8da0e12
Add BoundPolicyQuerySet to filter for enabled policy bindings related…
verkaufer Aug 4, 2024
836efed
Add flow test for unique password policy
verkaufer Aug 4, 2024
9f6a7d8
move PasswordPolicyUniquenessForm to its own module; rename to Unique…
verkaufer Aug 4, 2024
32cbdd9
fix: UniquePasswordPolicyViewset uses correct ModelViewSet class
verkaufer Aug 9, 2024
a3ac49e
Move UserPasswordHistory and store old password in CharField
verkaufer Aug 9, 2024
7676939
Move test for user_write
verkaufer Aug 9, 2024
61edb52
fix: Resolve schema conflict, UniquePasswordPolicy app now inherits f…
verkaufer Aug 17, 2024
4ff7b9b
fix: UniquePasswordPolicy form defaults numHistoricalPasswords to 1
verkaufer Aug 17, 2024
741c1bb
Merge branch 'main' into pr/10631
melizeche Mar 10, 2025
58a64b5
Fix imports and linting errors
melizeche Mar 10, 2025
50da439
Fix __str__ for tests
melizeche Mar 10, 2025
d4342d5
Add #nosec in tests to please bandit
melizeche Mar 10, 2025
c4b31fc
Merge branch 'main' into feature/unique_passwords
melizeche Mar 11, 2025
d62fa76
Improve error message
melizeche Mar 18, 2025
5681f4b
Merge branch 'main' into feature/unique_passwords
melizeche Mar 18, 2025
eeba846
update schema.yml
melizeche Mar 18, 2025
97c316f
fix policy not correctly checking bindings
BeryJu Mar 19, 2025
42ed2ee
Improve tests
melizeche Mar 19, 2025
d7c2c3d
Add more tests
melizeche Mar 19, 2025
e071160
Move policy in_use logic to the model, add tests
melizeche Mar 19, 2025
35c74a7
fix linting
melizeche Mar 19, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 21 additions & 10 deletions authentik/core/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -646,19 +646,30 @@ class SourceUserMatchingModes(models.TextChoices):
"""Different modes a source can handle new/returning users"""

IDENTIFIER = "identifier", _("Use the source-specific identifier")
EMAIL_LINK = "email_link", _(
"Link to a user with identical email address. Can have security implications "
"when a source doesn't validate email addresses."
EMAIL_LINK = (
"email_link",
_(
"Link to a user with identical email address. Can have security implications "
"when a source doesn't validate email addresses."
),
)
EMAIL_DENY = "email_deny", _(
"Use the user's email address, but deny enrollment when the email address already exists."
EMAIL_DENY = (
"email_deny",
_(
"Use the user's email address, but deny enrollment when the email address already "
"exists."
),
)
USERNAME_LINK = "username_link", _(
"Link to a user with identical username. Can have security implications "
"when a username is used with another source."
USERNAME_LINK = (
"username_link",
_(
"Link to a user with identical username. Can have security implications "
"when a username is used with another source."
),
)
USERNAME_DENY = "username_deny", _(
"Use the user's username, but deny enrollment when the username already exists."
USERNAME_DENY = (
"username_deny",
_("Use the user's username, but deny enrollment when the username already exists."),
)


Expand Down
5 changes: 4 additions & 1 deletion authentik/core/tests/test_tasks.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,10 @@
TokenIntents,
User,
)
from authentik.core.tasks import clean_expired_models, clean_temporary_users
from authentik.core.tasks import (
clean_expired_models,
clean_temporary_users,
)
from authentik.core.tests.utils import create_test_admin_user
from authentik.lib.generators import generate_id

Expand Down
6 changes: 5 additions & 1 deletion authentik/policies/apps.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,8 @@
"""authentik policies app config"""
"""Authentik policies app config

Every system policy should be its own Django app under the `policies` app.
For example: The 'dummy' policy is available at `authentik.policies.dummy`.
"""

from prometheus_client import Gauge, Histogram

Expand Down
10 changes: 10 additions & 0 deletions authentik/policies/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,13 @@ def supported_policy_binding_targets(self):
return ["policy", "user", "group"]


class BoundPolicyQuerySet(models.QuerySet):
"""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)


class PolicyBinding(SerializerModel):
"""Relationship between a Policy and a PolicyBindingModel."""

Expand Down Expand Up @@ -148,6 +155,9 @@ def __str__(self) -> str:
return f"Binding - #{self.order} to {suffix}"
return ""

objects = models.Manager()
in_use = BoundPolicyQuerySet.as_manager()

class Meta:
verbose_name = _("Policy Binding")
verbose_name_plural = _("Policy Bindings")
Expand Down
4 changes: 3 additions & 1 deletion authentik/policies/password/urls.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,4 +2,6 @@

from authentik.policies.password.api import PasswordPolicyViewSet

api_urlpatterns = [("policies/password", PasswordPolicyViewSet)]
api_urlpatterns = [
("policies/password", PasswordPolicyViewSet),
]
Empty file.
26 changes: 26 additions & 0 deletions authentik/policies/unique_password/api.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
from rest_framework.viewsets import ModelViewSet

from authentik.core.api.used_by import UsedByMixin
from authentik.policies.api.policies import PolicySerializer
from authentik.policies.unique_password.models import UniquePasswordPolicy


class UniquePasswordPolicySerializer(PolicySerializer):
"""Password Uniqueness Policy Serializer"""

class Meta:
model = UniquePasswordPolicy
fields = PolicySerializer.Meta.fields + [
"password_field",
"num_historical_passwords",
]


class UniquePasswordPolicyViewSet(UsedByMixin, ModelViewSet):
"""Password Uniqueness Policy Viewset"""

queryset = UniquePasswordPolicy.objects.all()
serializer_class = UniquePasswordPolicySerializer
filterset_fields = "__all__"
ordering = ["name"]
search_fields = ["name"]
10 changes: 10 additions & 0 deletions authentik/policies/unique_password/apps.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
"""authentik Unique Password policy app config"""

from authentik.blueprints.apps import ManagedAppConfig


class AuthentikPoliciesUniquePasswordConfig(ManagedAppConfig):
name = "authentik.policies.unique_password"
label = "authentik_policies_unique_password"
verbose_name = "authentik Policies.Unique Password"
default = True
79 changes: 79 additions & 0 deletions authentik/policies/unique_password/migrations/0001_initial.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
# Generated by Django 5.0.7 on 2024-08-09 02:49

import django.db.models.deletion
from django.conf import settings
from django.db import migrations, models


class Migration(migrations.Migration):

initial = True

dependencies = [
("authentik_policies", "0011_policybinding_failure_result_and_more"),
migrations.swappable_dependency(settings.AUTH_USER_MODEL),
]

operations = [
migrations.CreateModel(
name="UniquePasswordPolicy",
fields=[
(
"policy_ptr",
models.OneToOneField(
auto_created=True,
on_delete=django.db.models.deletion.CASCADE,
parent_link=True,
primary_key=True,
serialize=False,
to="authentik_policies.policy",
),
),
(
"password_field",
models.TextField(
default="password",
help_text="Field key to check, field keys defined in Prompt stages are available.",
),
),
(
"num_historical_passwords",
models.PositiveIntegerField(
default=0, help_text="Number of passwords to check against."
),
),
],
options={
"verbose_name": "Password Uniqueness Policy",
"verbose_name_plural": "Password Uniqueness Policies",
"indexes": [
models.Index(fields=["policy_ptr_id"], name="authentik_p_policy__f559aa_idx")
],
},
bases=("authentik_policies.policy",),
),
migrations.CreateModel(
name="UserPasswordHistory",
fields=[
(
"id",
models.AutoField(
auto_created=True, primary_key=True, serialize=False, verbose_name="ID"
),
),
("old_password", models.CharField(max_length=128)),
("created_at", models.DateTimeField(auto_now_add=True)),
(
"user",
models.ForeignKey(
on_delete=django.db.models.deletion.CASCADE,
related_name="old_passwords",
to=settings.AUTH_USER_MODEL,
),
),
],
options={
"verbose_name": "User Password History",
},
),
]
Empty file.
136 changes: 136 additions & 0 deletions authentik/policies/unique_password/models.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,136 @@
from django.contrib.auth.hashers import identify_hasher
from django.db import models
from django.utils.translation import gettext as _
from rest_framework.serializers import BaseSerializer
from structlog.stdlib import get_logger

from authentik.core.models import User
from authentik.policies.models import Policy
from authentik.policies.types import PolicyRequest, PolicyResult
from authentik.stages.prompt.stage import PLAN_CONTEXT_PROMPT

LOGGER = get_logger()


class UniquePasswordPolicy(Policy):
"""Policy ensuring a user's password is not identical to a previously used password.

After enabling the policy, Authentik stores every user's previous password whenever a user
changes their own password. Old passwords remain stored in hashed form.
"""

password_field = models.TextField(
default="password",
help_text=_("Field key to check, field keys defined in Prompt stages are available."),
)

# Limit on the number of previous passwords the policy evaluates
# Also controls number of old passwords the system stores.
num_historical_passwords = models.PositiveIntegerField(
default=0,
Copy link
Member

Choose a reason for hiding this comment

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

What does 0 mean in that case? Also it defaults to 1 in the frontend. If 0 is not valid, we should error on this.

help_text=_("Number of passwords to check against."),
)

@property
def serializer(self) -> type[BaseSerializer]:
from authentik.policies.unique_password.api import UniquePasswordPolicySerializer

return UniquePasswordPolicySerializer

@property
def component(self) -> str:
return "ak-policy-password-uniqueness-form"

def passes(self, request: PolicyRequest) -> PolicyResult:
from authentik.policies.unique_password.models import UserPasswordHistory

password = request.context.get(PLAN_CONTEXT_PROMPT, {}).get(
self.password_field, request.context.get(self.password_field)
)
if not password:
LOGGER.warning(
"Password field not found in request when checking UniquePasswordPolicy",
field=self.password_field,
fields=request.context.keys(),
)
return PolicyResult(False, _("Password not set in context"))
password = str(password)

if not self.num_historical_passwords:
# Policy not configured to check against any passwords
return PolicyResult(True)

Check warning on line 61 in authentik/policies/unique_password/models.py

View check run for this annotation

Codecov / codecov/patch

authentik/policies/unique_password/models.py#L61

Added line #L61 was not covered by tests

num_to_check = self.num_historical_passwords
password_history = UserPasswordHistory.objects.filter(user=request.user).order_by(
"-created_at"
)[:num_to_check]

if not password_history:
return PolicyResult(True)

for record in password_history:
if not record.old_password:
continue

Check warning on line 73 in authentik/policies/unique_password/models.py

View check run for this annotation

Codecov / codecov/patch

authentik/policies/unique_password/models.py#L73

Added line #L73 was not covered by tests

if self._passwords_match(new_password=password, old_password=record.old_password):
# Return on first match. Authentik does not consider timing attacks
# on old passwords to be an attack surface.
return PolicyResult(
False,
_("This password has been used previously. Please choose a different one."),
)

return PolicyResult(True)

def _passwords_match(self, *, new_password: str, old_password: str) -> bool:
try:
hasher = identify_hasher(old_password)
except ValueError:
LOGGER.warning(

Check warning on line 89 in authentik/policies/unique_password/models.py

View check run for this annotation

Codecov / codecov/patch

authentik/policies/unique_password/models.py#L88-L89

Added lines #L88 - L89 were not covered by tests
"Skipping password; could not load hash algorithm",
)
return False

Check warning on line 92 in authentik/policies/unique_password/models.py

View check run for this annotation

Codecov / codecov/patch

authentik/policies/unique_password/models.py#L92

Added line #L92 was not covered by tests

return hasher.verify(new_password, old_password)

@classmethod
def is_in_use(cls):
"""Check if any UniquePasswordPolicy is in use, either through policy bindings
or direct attachment to a PromptStage.

Returns:
bool: True if any policy is in use, False otherwise
"""
from authentik.policies.models import PolicyBinding

# Check if any policy is in use through bindings
if PolicyBinding.in_use.for_policy(cls).exists():
return True

# Check if any policy is attached to a PromptStage
if cls.objects.filter(promptstage__isnull=False).exists():
return True

return False

class Meta(Policy.PolicyMeta):
verbose_name = _("Password Uniqueness Policy")
verbose_name_plural = _("Password Uniqueness Policies")


class UserPasswordHistory(models.Model):
user = models.ForeignKey(User, on_delete=models.CASCADE, related_name="old_passwords")
# Mimic's column type of AbstractBaseUser.password
old_password = models.CharField(max_length=128)
created_at = models.DateTimeField(auto_now_add=True)

class Meta:
verbose_name = _("User Password History")

def __str__(self) -> str:
if self.created_at:
Copy link
Member

Choose a reason for hiding this comment

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

This should never be null.

return (
f"Previous Password (user: {self.user_id}, "
f"recorded: {self.created_at:%Y/%m/%d %X})"
)
return f"Previous Password (user: {self.user_id}, recorded: None)"
49 changes: 49 additions & 0 deletions authentik/policies/unique_password/signals.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
"""authentik policy signals"""

from typing import Any

from django.db.models.signals import post_delete
from django.dispatch import receiver
from django.http import HttpRequest

from authentik.core.middleware import SESSION_KEY_IMPERSONATE_USER
from authentik.core.models import User
from authentik.policies.models import PolicyBinding
from authentik.policies.unique_password.tasks import (
purge_password_history_table,
trim_user_password_history,
)
from authentik.stages.user_write.signals import user_write


@receiver(post_delete, sender=PolicyBinding)
def purge_password_history(sender, instance, **_):
from authentik.policies.unique_password.models import UniquePasswordPolicy
Copy link
Member

Choose a reason for hiding this comment

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

Do we have to have this as a JIT import?


if not isinstance(instance.policy, UniquePasswordPolicy):
return
purge_password_history_table.delay()
Copy link
Member

Choose a reason for hiding this comment

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

We may (?) want this to run on a schedule rather than triggering it here.



@receiver(user_write)
Copy link
Member

Choose a reason for hiding this comment

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

There is a password_changed signal you can listen on. It's sent from authentik/core/models.py::User::set_password

Copy link
Member

Choose a reason for hiding this comment

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

It will also allow you to ignore password changes in case request.user != user, in which case the user's password was changed by an admin from the admin interface, and should probably (?) be ignored

def copy_password_to_password_history(
sender, request: HttpRequest, user: User, data: dict[str, Any], **kwargs
):
"""Preserve the user's old password if UniquePasswordPolicy is enabled anywhere"""
from authentik.policies.unique_password.models import UniquePasswordPolicy, UserPasswordHistory

user_changed_own_password = (
any("password" in x for x in data.keys())
and request.user.pk == user.pk
and SESSION_KEY_IMPERSONATE_USER not in request.session
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we should have a special case for impersonation here. In my mind, this is happening in a password change flow, and should run the same way on impersonation than on normal runs. If the admin wants to change a password for a user without going through all the checks, they can still do so from the admin interface directly.

)
if user_changed_own_password:
# Check if any UniquePasswordPolicy is in use
unique_pwd_policy_in_use = UniquePasswordPolicy.is_in_use()

if unique_pwd_policy_in_use:
"""NOTE: Because we run this in a signal after saving the user,
we are not atomically guaranteed to save password history.
"""
UserPasswordHistory.objects.create(user=user, old_password=user.password)
trim_user_password_history.delay(user.pk)
Copy link
Member

Choose a reason for hiding this comment

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

We probably want this to run on a schedule rather than triggering it here.

Loading
Loading