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.

Sign up for GitHub

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/reputation: save to database directly #10059

Merged
merged 5 commits into from
Jun 14, 2024
Merged

policies/reputation: save to database directly #10059

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f7f0be6
policies/reputation: save to database directly
rissson Jun 10, 2024
9603042
makemigrations
rissson Jun 11, 2024
e756aac
fix settings
rissson Jun 11, 2024
3838dee
also update expiry
rissson Jun 11, 2024
b5dd60b
lint?
rissson Jun 12, 2024
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
1 change: 0 additions & 1 deletion authentik/lib/default.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,6 @@ cache:
timeout: 300
timeout_flows: 300
timeout_policies: 300
timeout_reputation: 300
BeryJu marked this conversation as resolved.
Show resolved Hide resolved

# channel:
# url: ""
Expand Down
2 changes: 0 additions & 2 deletions authentik/policies/reputation/apps.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,6 @@

from authentik.blueprints.apps import ManagedAppConfig

CACHE_KEY_PREFIX = "goauthentik.io/policies/reputation/scores/"


class AuthentikPolicyReputationConfig(ManagedAppConfig):
"""Authentik reputation app config"""
Expand Down
25 changes: 25 additions & 0 deletions ...policies/reputation/migrations/0007_reputation_authentik_p_identif_9434d7_idx_and_more.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
# Generated by Django 5.0.6 on 2024-06-11 08:50

from django.db import migrations, models


class Migration(migrations.Migration):

dependencies = [
("authentik_policies_reputation", "0006_reputation_ip_asn_data"),
]

operations = [
migrations.AddIndex(
model_name="reputation",
index=models.Index(fields=["identifier"], name="authentik_p_identif_9434d7_idx"),
),
migrations.AddIndex(
model_name="reputation",
index=models.Index(fields=["ip"], name="authentik_p_ip_7ad0df_idx"),
),
migrations.AddIndex(
model_name="reputation",
index=models.Index(fields=["ip", "identifier"], name="authentik_p_ip_d779aa_idx"),
),
]
5 changes: 5 additions & 0 deletions authentik/policies/reputation/models.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,3 +96,8 @@ class Meta:
verbose_name = _("Reputation Score")
verbose_name_plural = _("Reputation Scores")
unique_together = ("identifier", "ip")
indexes = [
models.Index(fields=["identifier"]),
models.Index(fields=["ip"]),
models.Index(fields=["ip", "identifier"]),
]
Comment on lines +99 to +103
Copy link
Member Author

Choose a reason for hiding this comment

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

Not fully sure about those either, but it should help a bit

11 changes: 0 additions & 11 deletions authentik/policies/reputation/settings.py
View file
Open in desktop

This file was deleted.

31 changes: 13 additions & 18 deletions authentik/policies/reputation/signals.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,40 +1,35 @@
"""authentik reputation request signals"""

from django.contrib.auth.signals import user_logged_in
from django.core.cache import cache
from django.dispatch import receiver
from django.http import HttpRequest
from structlog.stdlib import get_logger

from authentik.core.signals import login_failed
from authentik.lib.config import CONFIG
from authentik.policies.reputation.apps import CACHE_KEY_PREFIX
from authentik.policies.reputation.tasks import save_reputation
from authentik.events.context_processors.asn import ASN_CONTEXT_PROCESSOR
from authentik.events.context_processors.geoip import GEOIP_CONTEXT_PROCESSOR
from authentik.policies.reputation.models import Reputation, reputation_expiry
from authentik.root.middleware import ClientIPMiddleware
from authentik.stages.identification.signals import identification_failed

LOGGER = get_logger()
CACHE_TIMEOUT = CONFIG.get_int("cache.timeout_reputation")


def update_score(request: HttpRequest, identifier: str, amount: int):
"""Update score for IP and User"""
remote_ip = ClientIPMiddleware.get_client_ip(request)

try:
# We only update the cache here, as its faster than writing to the DB
score = cache.get_or_set(
CACHE_KEY_PREFIX + remote_ip + "/" + identifier,
{"ip": remote_ip, "identifier": identifier, "score": 0},
CACHE_TIMEOUT,
)
score["score"] += amount
cache.set(CACHE_KEY_PREFIX + remote_ip + "/" + identifier, score)
except ValueError as exc:
LOGGER.warning("failed to set reputation", exc=exc)

Reputation.objects.update_or_create(
ip=remote_ip,
identifier=identifier,
defaults={
"score": amount,
"ip_geo_data": GEOIP_CONTEXT_PROCESSOR.city_dict(remote_ip) or {},
Copy link
Member

Choose a reason for hiding this comment

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

I think both this and ip_asn_data we should maybe dynamically fetch instead of storing in it in the database as the lookup can take quite a bit of time? And then we can cache results of GEOIP_CONTEXT_PROCESSOR based on the IP within the process? (or in redis..?) (maybe this is premature optimization)

Copy link
Member Author

Choose a reason for hiding this comment

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

Everything should be in memory so it should be quite fast, maybe even faster than storing things in Redis.

"ip_asn_data": ASN_CONTEXT_PROCESSOR.asn_dict(remote_ip) or {},
"expires": reputation_expiry(),
},
)
Copy link
Member

Choose a reason for hiding this comment

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

We'll also need to update the expiry timestamp here based on the setting above...and maybe wrap this call in a transaction/catch errors as any saving here would prevent logins from happening due to this being in a sync signal

Copy link
Member Author

Choose a reason for hiding this comment

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

Weird that the expiry wasn't updated previously.

Copy link
Member Author

Choose a reason for hiding this comment

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

As for the catching exception, the previous code didn't and I think it's a good thing, as if we can't save the reputation score anywhere, you shouldn't be able to login as that data can be used for security purposes.

LOGGER.debug("Updated score", amount=amount, for_user=identifier, for_ip=remote_ip)
save_reputation.delay()


@receiver(login_failed)
Expand Down
32 changes: 0 additions & 32 deletions authentik/policies/reputation/tasks.py
View file
Open in desktop

This file was deleted.

19 changes: 0 additions & 19 deletions authentik/policies/reputation/tests.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,11 @@
"""test reputation signals and policy"""

from django.core.cache import cache
from django.test import RequestFactory, TestCase

from authentik.core.models import User
from authentik.lib.generators import generate_id
from authentik.policies.reputation.api import ReputationPolicySerializer
from authentik.policies.reputation.apps import CACHE_KEY_PREFIX
from authentik.policies.reputation.models import Reputation, ReputationPolicy
from authentik.policies.reputation.tasks import save_reputation
from authentik.policies.types import PolicyRequest
from authentik.stages.password import BACKEND_INBUILT
from authentik.stages.password.stage import authenticate
Expand All @@ -22,8 +19,6 @@ def setUp(self):
self.request = self.request_factory.get("/")
self.test_ip = "127.0.0.1"
self.test_username = "test"
keys = cache.keys(CACHE_KEY_PREFIX + "*")
cache.delete_many(keys)
# We need a user for the one-to-one in userreputation
self.user = User.objects.create(username=self.test_username)
self.backends = [BACKEND_INBUILT]
Expand All @@ -34,13 +29,6 @@ def test_ip_reputation(self):
authenticate(
self.request, self.backends, username=self.test_username, password=self.test_username
)
# Test value in cache
self.assertEqual(
cache.get(CACHE_KEY_PREFIX + self.test_ip + "/" + self.test_username),
{"ip": "127.0.0.1", "identifier": "test", "score": -1},
)
# Save cache and check db values
save_reputation.delay().get()
self.assertEqual(Reputation.objects.get(ip=self.test_ip).score, -1)

def test_user_reputation(self):
Expand All @@ -49,13 +37,6 @@ def test_user_reputation(self):
authenticate(
self.request, self.backends, username=self.test_username, password=self.test_username
)
# Test value in cache
self.assertEqual(
cache.get(CACHE_KEY_PREFIX + self.test_ip + "/" + self.test_username),
{"ip": "127.0.0.1", "identifier": "test", "score": -1},
)
# Save cache and check db values
save_reputation.delay().get()
self.assertEqual(Reputation.objects.get(identifier=self.test_username).score, -1)

def test_policy(self):
Expand Down
Loading