Skip to content

Commit

Permalink
fix(asm): login failure events user exists field optional (#8742)
Browse files Browse the repository at this point in the history
ASM: This fix resolves an issue where django login failure events may
send wrong information of user existence.

## Checklist

- [x] Change(s) are motivated and described in the PR description
- [x] Testing strategy is described if automated tests are not included
in the PR
- [x] Risks are described (performance impact, potential for breakage,
maintainability)
- [x] Change is maintainable (easy to change, telemetry, documentation)
- [x] [Library release note
guidelines](https://ddtrace.readthedocs.io/en/stable/releasenotes.html)
are followed or label `changelog/no-changelog` is set
- [x] Documentation is included (in-code, generated user docs, [public
corp docs](https://github.com/DataDog/documentation/))
- [x] Backport labels are set (if
[applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting))
- [x] If this PR changes the public interface, I've notified
`@DataDog/apm-tees`.
- [x] If change touches code that signs or publishes builds or packages,
or handles credentials of any kind, I've requested a review from
`@DataDog/security-design-and-guidance`.

## Reviewer Checklist

- [x] Title is accurate
- [x] All changes are related to the pull request's stated goal
- [x] Description motivates each change
- [x] Avoids breaking
[API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces)
changes
- [x] Testing strategy adequately addresses listed risks
- [x] Change is maintainable (easy to change, telemetry, documentation)
- [x] Release note makes sense to a user of the library
- [x] Author has acknowledged and discussed the performance implications
of this PR as reported in the benchmarks PR comment
- [x] Backport labels are set in a manner that is consistent with the
[release branch maintenance
policy](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)
  • Loading branch information
gnufede authored Mar 22, 2024
1 parent 4e17f52 commit d953682
Show file tree
Hide file tree
Showing 3 changed files with 35 additions and 7 deletions.
14 changes: 8 additions & 6 deletions ddtrace/appsec/_trace_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@ def track_user_login_success_event(
def track_user_login_failure_event(
tracer: Tracer,
user_id: Optional[str],
exists: bool,
exists: Optional[bool] = None,
metadata: Optional[dict] = None,
login_events_mode: str = LOGIN_EVENTS_MODE.SDK,
) -> None:
Expand All @@ -152,8 +152,10 @@ def track_user_login_failure_event(

if user_id:
span.set_tag_str("%s.failure.%s" % (APPSEC.USER_LOGIN_EVENT_PREFIX_PUBLIC, user.ID), str(user_id))
exists_str = "true" if exists else "false"
span.set_tag_str("%s.failure.%s" % (APPSEC.USER_LOGIN_EVENT_PREFIX_PUBLIC, user.EXISTS), exists_str)

if exists is not None:
exists_str = "true" if exists else "false"
span.set_tag_str("%s.failure.%s" % (APPSEC.USER_LOGIN_EVENT_PREFIX_PUBLIC, user.EXISTS), exists_str)


def track_user_signup_event(
Expand Down Expand Up @@ -312,9 +314,9 @@ def _on_django_login(
**user_extra,
)
else:
# Login failed and the user is unknown
# Login failed and the user is unknown (may exist or not)
user_id = info_retriever.get_userid()
track_user_login_failure_event(pin.tracer, user_id=user_id, exists=False, login_events_mode=mode)
track_user_login_failure_event(pin.tracer, user_id=user_id, login_events_mode=mode)


def _on_django_auth(result_user, mode, kwargs, pin, info_retriever):
Expand All @@ -333,7 +335,7 @@ def _on_django_auth(result_user, mode, kwargs, pin, info_retriever):

if not result_user:
with pin.tracer.trace("django.contrib.auth.login", span_type=SpanTypes.AUTH):
track_user_login_failure_event(pin.tracer, user_id=user_id, exists=False, login_events_mode=mode)
track_user_login_failure_event(pin.tracer, user_id=user_id, login_events_mode=mode)

return False, None

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
fixes:
- |
ASM: This fix resolves an issue where django login failure events may send wrong information of user existence.
23 changes: 22 additions & 1 deletion tests/contrib/django/test_django_appsec.py
Original file line number Diff line number Diff line change
Expand Up @@ -239,7 +239,28 @@ def test_django_login_failure_user_doesnt_exists(client, test_spans, tracer):
login_span = test_spans.find_span(name="django.contrib.auth.login")
assert login_span.get_tag("appsec.events.users.login.failure.track") == "true"
assert login_span.get_tag(APPSEC.USER_LOGIN_EVENT_PREFIX_PUBLIC + ".failure." + user.ID) == "missing"
assert login_span.get_tag(APPSEC.USER_LOGIN_EVENT_PREFIX_PUBLIC + ".failure." + user.EXISTS) == "false"
## TODO: Disabled until we have a proper way to detect whether the user exists
# assert login_span.get_tag(APPSEC.USER_LOGIN_EVENT_PREFIX_PUBLIC + ".failure." + user.EXISTS) == "false"
assert login_span.get_tag(APPSEC.AUTO_LOGIN_EVENTS_FAILURE_MODE) == "extended"


@pytest.mark.django_db
def test_django_login_failure_user_does_exist(client, test_spans, tracer):
from django.contrib.auth import get_user
from django.contrib.auth.models import User

with override_global_config(dict(_asm_enabled=True, _automatic_login_events_mode="extended")):
test_user = User.objects.create(username="fred")
test_user.set_password("secret")
test_user.save()
assert not get_user(client).is_authenticated
client.login(username="fred", password="wrong")
assert not get_user(client).is_authenticated
login_span = test_spans.find_span(name="django.contrib.auth.login")
assert login_span.get_tag("appsec.events.users.login.failure.track") == "true"
assert login_span.get_tag(APPSEC.USER_LOGIN_EVENT_PREFIX_PUBLIC + ".failure." + user.ID) == "fred"
## TODO: Disabled until we have a proper way to detect whether the user exists
# assert login_span.get_tag(APPSEC.USER_LOGIN_EVENT_PREFIX_PUBLIC + ".failure." + user.EXISTS) == "true"
assert login_span.get_tag(APPSEC.AUTO_LOGIN_EVENTS_FAILURE_MODE) == "extended"


Expand Down

0 comments on commit d953682

Please sign in to comment.