fix(asm): put back email/login/name in traces for django user (#11288)…

… [backport 2.16] (#11333) Backport to 2.16 of #11288 ---- APPSEC-55708 APMS-13572 - Add `DD_DJANGO_INCLUDE_USER_EMAIL` (default to false), `DD_DJANGO_INCLUDE_USER_LOGIN` (default to true) and `DD_DJANGO_INCLUDE_USER_REALNAME` (default to false) to specifically add email/login/real_name to the span tags if user event mode is "identification" - Add `APPSEC_AUTO_EVENTS_EXTENDED` system tests scenario to CI (to test that feature) - Update appropriate tests - Update Django integration documentation ---- ## Checklist - [x] PR author has checked that all the criteria below are met - The PR description includes an overview of the change - The PR description articulates the motivation for the change - The change includes tests OR the PR description describes a testing strategy - The PR description notes risks associated with the change, if any - Newly-added code is easy to change - The change follows the [library release note guidelines](https://ddtrace.readthedocs.io/en/stable/releasenotes.html) - The change includes or references documentation updates if necessary - Backport labels are set (if [applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)) ## Reviewer Checklist - [x] Reviewer has checked that all the criteria below are met - Title is accurate - All changes are related to the pull request's stated goal - Avoids breaking [API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces) changes - Testing strategy adequately addresses listed risks - Newly-added code is easy to change - Release note makes sense to a user of the library - If necessary, author has acknowledged and discussed the performance implications of this PR as reported in the benchmarks PR comment - 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)
DataDog · Nov 8, 2024 · 50ba4c9 · 50ba4c9
1 parent ecee3b9
commit 50ba4c9
Show file tree

Hide file tree

Showing 10 changed files with 148 additions and 30 deletions.
diff --git a/.github/workflows/system-tests.yml b/.github/workflows/system-tests.yml
@@ -165,6 +165,10 @@ jobs:
         if: always() && steps.docker_load.outcome == 'success' && matrix.scenario == 'appsec'
         run: ./run.sh APPSEC_MISSING_RULES
 
+      - name: Run APPSEC_AUTO_EVENTS_EXTENDED
+        if: always() && steps.docker_load.outcome == 'success' && matrix.scenario == 'appsec'
+        run: ./run.sh APPSEC_AUTO_EVENTS_EXTENDED
+
       - name: Run APPSEC_CUSTOM_RULES
         if: always() && steps.docker_load.outcome == 'success' && matrix.scenario == 'appsec'
         run: ./run.sh APPSEC_CUSTOM_RULES

diff --git a/ddtrace/appsec/_trace_utils.py b/ddtrace/appsec/_trace_utils.py
@@ -117,6 +117,8 @@ def track_user_login_success_event(
     real_mode = login_events_mode if login_events_mode != LOGIN_EVENTS_MODE.AUTO else asm_config._user_event_mode
     if real_mode == LOGIN_EVENTS_MODE.DISABLED:
         return
+    if real_mode == LOGIN_EVENTS_MODE.ANON:
+        login = name = email = None
     span = _track_user_login_common(tracer, True, metadata, login_events_mode, login, name, email, span)
     if not span:
         return
@@ -293,13 +295,17 @@ def _on_django_login(
     user,
     mode,
     info_retriever,
+    django_config,
 ):
     if user:
         from ddtrace.contrib.django.compat import user_is_authenticated
 
         if user_is_authenticated(user):
-            user_id = info_retriever.get_userid()
-
+            user_id, user_extra = info_retriever.get_user_info(
+                login=django_config.include_user_login,
+                email=django_config.include_user_email,
+                name=django_config.include_user_realname,
+            )
             with pin.tracer.trace("django.contrib.auth.login", span_type=SpanTypes.AUTH):
                 session_key = getattr(request, "session_key", None)
                 track_user_login_success_event(
@@ -308,14 +314,15 @@ def _on_django_login(
                     session_id=session_key,
                     propagate=True,
                     login_events_mode=mode,
+                    **user_extra,
                 )
         else:
             # 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, login_events_mode=mode)
 
 
-def _on_django_auth(result_user, mode, kwargs, pin, info_retriever):
+def _on_django_auth(result_user, mode, kwargs, pin, info_retriever, django_config):
     if not asm_config._asm_enabled:
         return True, result_user
 
@@ -332,12 +339,13 @@ def _on_django_auth(result_user, mode, kwargs, pin, info_retriever):
         with pin.tracer.trace("django.contrib.auth.login", span_type=SpanTypes.AUTH):
             exists = info_retriever.user_exists()
             if exists:
-                user_id = info_retriever.get_userid()
+                user_id, user_extra = info_retriever.get_user_info(
+                    login=django_config.include_user_login,
+                    email=django_config.include_user_email,
+                    name=django_config.include_user_realname,
+                )
                 track_user_login_failure_event(
-                    pin.tracer,
-                    user_id=user_id,
-                    login_events_mode=mode,
-                    exists=True,
+                    pin.tracer, user_id=user_id, login_events_mode=mode, exists=True, **user_extra
                 )
             else:
                 track_user_login_failure_event(pin.tracer, user_id=user_id, login_events_mode=mode, exists=False)

diff --git a/ddtrace/appsec/_utils.py b/ddtrace/appsec/_utils.py
@@ -145,7 +145,7 @@ def get_name(self):
 
         return self.find_in_user_model(self.possible_name_fields)
 
-    def get_user_info(self):
+    def get_user_info(self, login=False, email=False, name=False):
         """
         In safe mode, try to get the user id from the user object.
         In extended mode, try to also get the username (which will be the returned user_id),
@@ -157,12 +157,12 @@ def get_user_info(self):
         if not user_id:
             return None, {}
 
-        user_extra_info = {
-            "login": self.get_username(),
-            "email": self.get_user_email(),
-            "name": self.get_name(),
-        }
-
+        if login:
+            user_extra_info["login"] = self.get_username()
+        if email:
+            user_extra_info["email"] = self.get_user_email()
+        if name:
+            user_extra_info["name"] = self.get_name()
         return user_id, user_extra_info
 
 

diff --git a/ddtrace/contrib/django/__init__.py b/ddtrace/contrib/django/__init__.py
@@ -125,12 +125,36 @@
 
 .. py:data:: ddtrace.config.django['include_user_name']
 
-   Whether or not to include the authenticated user's username as a tag on the root request span.
+   Whether or not to include the authenticated user's name/id as a tag on the root request span.
 
    Can also be configured via the ``DD_DJANGO_INCLUDE_USER_NAME`` environment variable.
 
    Default: ``True``
 
+.. py:data:: ddtrace.config.django['include_user_email']
+
+   (ASM) Whether or not to include the authenticated user's email (if available) as a tag on the root request span on a user event.
+
+   Can also be configured via the ``DD_DJANGO_INCLUDE_USER_EMAIL`` environment variable.
+
+   Default: ``False``
+
+.. py:data:: ddtrace.config.django['include_user_login']
+
+   (ASM) Whether or not to include the authenticated user's login (if available) as a tag on the root request span on a user event.
+
+   Can also be configured via the ``DD_DJANGO_INCLUDE_USER_LOGIN`` environment variable.
+
+   Default: ``True``
+
+.. py:data:: ddtrace.config.django['include_user_realname']
+
+   (ASM) Whether or not to include the authenticated user's real name (if available) as a tag on the root request span on a user event.
+
+   Can also be configured via the ``DD_DJANGO_INCLUDE_USER_REALNAME`` environment variable.
+
+   Default: ``False``
+
 .. py:data:: ddtrace.config.django['use_handler_resource_format']
 
    Whether or not to use the resource format `"{method} {handler}"`. Can also be
@@ -176,6 +200,7 @@
 
 .. __: https://www.djangoproject.com/
 """  # noqa: E501
+
 from ddtrace.internal.utils.importlib import require_modules
 
 

diff --git a/ddtrace/contrib/internal/django/patch.py b/ddtrace/contrib/internal/django/patch.py
@@ -70,7 +70,10 @@
         analytics_enabled=None,  # None allows the value to be overridden by the global config
         analytics_sample_rate=None,
         trace_query_string=None,  # Default to global config
-        include_user_name=asbool(os.getenv("DD_DJANGO_INCLUDE_USER_NAME", default=True)),
+        include_user_name=asm_config._django_include_user_name,
+        include_user_email=asm_config._django_include_user_email,
+        include_user_login=asm_config._django_include_user_login,
+        include_user_realname=asm_config._django_include_user_realname,
         use_handler_with_url_name_resource_format=asbool(
             os.getenv("DD_DJANGO_USE_HANDLER_WITH_URL_NAME_RESOURCE_FORMAT", default=False)
         ),
@@ -781,7 +784,7 @@ def traced_login(django, pin, func, instance, args, kwargs):
     try:
         request = get_argument_value(args, kwargs, 0, "request")
         user = get_argument_value(args, kwargs, 1, "user")
-        core.dispatch("django.login", (pin, request, user, mode, _DjangoUserInfoRetriever(user)))
+        core.dispatch("django.login", (pin, request, user, mode, _DjangoUserInfoRetriever(user), config.django))
     except Exception:
         log.debug("Error while trying to trace Django login", exc_info=True)
 
@@ -794,7 +797,8 @@ def traced_authenticate(django, pin, func, instance, args, kwargs):
         return result_user
     try:
         result = core.dispatch_with_results(
-            "django.auth", (result_user, mode, kwargs, pin, _DjangoUserInfoRetriever(result_user, credentials=kwargs))
+            "django.auth",
+            (result_user, mode, kwargs, pin, _DjangoUserInfoRetriever(result_user, credentials=kwargs), config.django),
         ).user
         if result and result.value[0]:
             return result.value[1]

diff --git a/ddtrace/contrib/internal/django/utils.py b/ddtrace/contrib/internal/django/utils.py
@@ -317,7 +317,6 @@ def _after_request_tags(pin, span: Span, request, response):
     # Response can be None in the event that the request failed
     # We still want to set additional request tags that are resolved
     # during the request.
-
     try:
         user = getattr(request, "user", None)
         if user is not None:

diff --git a/ddtrace/settings/asm.py b/ddtrace/settings/asm.py
@@ -170,6 +170,12 @@ class ASMConfig(Env):
         int, EXPLOIT_PREVENTION.MAX_STACK_TRACE_DEPTH, default=32, validator=_validate_non_negative_int
     )
 
+    # Django ATO
+    _django_include_user_name = Env.var(bool, "DD_DJANGO_INCLUDE_USER_NAME", default=True)
+    _django_include_user_email = Env.var(bool, "DD_DJANGO_INCLUDE_USER_EMAIL", default=False)
+    _django_include_user_login = Env.var(bool, "DD_DJANGO_INCLUDE_USER_LOGIN", default=True)
+    _django_include_user_realname = Env.var(bool, "DD_DJANGO_INCLUDE_USER_REALNAME", default=False)
+
     # for tests purposes
     _asm_config_keys = [
         "_asm_enabled",
@@ -206,6 +212,10 @@ class ASMConfig(Env):
         "_ep_max_stack_trace_depth",
         "_asm_config_keys",
         "_deduplication_enabled",
+        "_django_include_user_name",
+        "_django_include_user_email",
+        "_django_include_user_login",
+        "_django_include_user_realname",
     ]
     _iast_redaction_numeral_pattern = Env.var(
         str,

diff --git a/releasenotes/notes/ato_fix__feature_flag_for_optional_user_info-28a1ce33eeb6a275.yaml b/releasenotes/notes/ato_fix__feature_flag_for_optional_user_info-28a1ce33eeb6a275.yaml
@@ -0,0 +1,7 @@
+---
+fixes:
+  - |
+    ASM: The new user events policy is preventing users PII to be added by default as span tags. To allow customers using
+    the Django auto instrumentation to still have those information, new environment variables have been added.
+    In particular DD_DJANGO_INCLUDE_EMAIL (false by default), will tag user events with user email as before.
+
diff --git a/tests/contrib/django/test_django_appsec.py b/tests/contrib/django/test_django_appsec.py
@@ -1,7 +1,10 @@
 # -*- coding: utf-8 -*-
 
+import contextlib
+
 import pytest
 
+from ddtrace import config
 from ddtrace.appsec._constants import APPSEC
 from ddtrace.appsec._constants import LOGIN_EVENTS_MODE
 from ddtrace.appsec._constants import SPAN_DATA_NAMES
@@ -14,6 +17,24 @@
 from tests.utils import override_global_config
 
 
+@contextlib.contextmanager
+def update_django_config():
+    initial_settings = (
+        config.django.include_user_email,
+        config.django.include_user_login,
+        config.django.include_user_realname,
+    )
+    config.django.include_user_email = asm_config._django_include_user_email
+    config.django.include_user_login = asm_config._django_include_user_login
+    config.django.include_user_realname = asm_config._django_include_user_realname
+    yield
+    (
+        config.django.include_user_email,
+        config.django.include_user_login,
+        config.django.include_user_realname,
+    ) = initial_settings
+
+
 def _aux_appsec_get_root_span(
     client,
     test_spans,
@@ -142,11 +163,23 @@ def test_django_login_events_disabled_noappsec(client, test_spans, tracer):
 
 
 @pytest.mark.django_db
-def test_django_login_sucess_identification(client, test_spans, tracer):
+@pytest.mark.parametrize("use_login", (False, True))
+@pytest.mark.parametrize("use_email", (False, True))
+@pytest.mark.parametrize("use_realname", (False, True))
+def test_django_login_sucess_identification(client, test_spans, tracer, use_login, use_email, use_realname):
     from django.contrib.auth import get_user
     from django.contrib.auth.models import User
 
-    with override_global_config(dict(_asm_enabled=True, _auto_user_instrumentation_local_mode=LOGIN_EVENTS_MODE.IDENT)):
+    with override_global_config(
+        dict(
+            _asm_enabled=True,
+            _auto_user_instrumentation_local_mode=LOGIN_EVENTS_MODE.IDENT,
+            _django_include_user_email=use_email,
+            _django_include_user_login=use_login,
+            _django_include_user_realname=use_realname,
+        )
+    ), update_django_config():
+        # update django config for tests
         test_user = User.objects.create(username="fred", first_name="Fred", email="fred@test.com")
         test_user.set_password("secret")
         test_user.save()
@@ -158,17 +191,41 @@ def test_django_login_sucess_identification(client, test_spans, tracer):
         assert login_span.get_tag(user.ID) == "1"
         assert login_span.get_tag(APPSEC.USER_LOGIN_EVENT_PREFIX_PUBLIC + ".success.track") == "true"
         assert login_span.get_tag(APPSEC.AUTO_LOGIN_EVENTS_SUCCESS_MODE) == LOGIN_EVENTS_MODE.IDENT
-        assert login_span.get_tag(APPSEC.USER_LOGIN_EVENT_PREFIX + ".success.login") is None
-        assert login_span.get_tag(APPSEC.USER_LOGIN_EVENT_PREFIX + ".success.email") is None
-        assert login_span.get_tag(APPSEC.USER_LOGIN_EVENT_PREFIX + ".success.username") is None
+        if use_login:
+            assert login_span.get_tag(APPSEC.USER_LOGIN_EVENT_PREFIX + ".success.login") == "fred"
+        else:
+            assert login_span.get_tag(APPSEC.USER_LOGIN_EVENT_PREFIX + ".success.login") is None
+        if use_email:
+            assert login_span.get_tag(APPSEC.USER_LOGIN_EVENT_PREFIX + ".success.email") == "fred@test.com"
+            assert login_span.get_tag("usr.email") == "fred@test.com"
+        else:
+            assert login_span.get_tag(APPSEC.USER_LOGIN_EVENT_PREFIX + ".success.email") is None
+            assert login_span.get_tag("usr.email") is None
+        if use_realname:
+            assert login_span.get_tag(APPSEC.USER_LOGIN_EVENT_PREFIX + ".success.username") == "Fred"
+            assert login_span.get_tag("usr.name") == "Fred"
+        else:
+            assert login_span.get_tag(APPSEC.USER_LOGIN_EVENT_PREFIX + ".success.username") is None
+            assert login_span.get_tag("usr.name") is None
 
 
 @pytest.mark.django_db
-def test_django_login_sucess_anonymization(client, test_spans, tracer):
+@pytest.mark.parametrize("use_login", (False, True))
+@pytest.mark.parametrize("use_email", (False, True))
+@pytest.mark.parametrize("use_realname", (False, True))
+def test_django_login_sucess_anonymization(client, test_spans, tracer, use_login, use_email, use_realname):
     from django.contrib.auth import get_user
     from django.contrib.auth.models import User
 
-    with override_global_config(dict(_asm_enabled=True, _auto_user_instrumentation_local_mode=LOGIN_EVENTS_MODE.ANON)):
+    with override_global_config(
+        dict(
+            _asm_enabled=True,
+            _auto_user_instrumentation_local_mode=LOGIN_EVENTS_MODE.ANON,
+            _django_include_user_email=use_email,
+            _django_include_user_login=use_login,
+            _django_include_user_realname=use_realname,
+        )
+    ), update_django_config():
         test_user = User.objects.create(username="fred2")
         test_user.set_password("secret")
         test_user.save()
@@ -180,9 +237,9 @@ def test_django_login_sucess_anonymization(client, test_spans, tracer):
         assert login_span.get_tag(user.ID) == "1"
         assert login_span.get_tag("appsec.events.users.login.success.track") == "true"
         assert login_span.get_tag(APPSEC.AUTO_LOGIN_EVENTS_SUCCESS_MODE) == LOGIN_EVENTS_MODE.ANON
-        assert not login_span.get_tag(APPSEC.USER_LOGIN_EVENT_PREFIX + ".success.login")
-        assert not login_span.get_tag(APPSEC.USER_LOGIN_EVENT_PREFIX_PUBLIC + ".success.email")
-        assert not login_span.get_tag(APPSEC.USER_LOGIN_EVENT_PREFIX_PUBLIC + ".success.username")
+        assert login_span.get_tag(APPSEC.USER_LOGIN_EVENT_PREFIX + ".success.login") is None
+        assert login_span.get_tag(APPSEC.USER_LOGIN_EVENT_PREFIX + ".success.email") is None
+        assert login_span.get_tag(APPSEC.USER_LOGIN_EVENT_PREFIX + ".success.username") is None
 
 
 @pytest.mark.django_db

diff --git a/tests/telemetry/test_writer.py b/tests/telemetry/test_writer.py
@@ -333,6 +333,10 @@ def test_app_started_event_configuration_override(test_agent_session, run_python
         {"name": "DD_CRASHTRACKING_TAGS", "origin": "default", "value": ""},
         {"name": "DD_CRASHTRACKING_WAIT_FOR_RECEIVER", "origin": "default", "value": True},
         {"name": "DD_DATA_STREAMS_ENABLED", "origin": "env_var", "value": True},
+        {"name": "DD_DJANGO_INCLUDE_USER_EMAIL", "origin": "default", "value": False},
+        {"name": "DD_DJANGO_INCLUDE_USER_LOGIN", "origin": "default", "value": True},
+        {"name": "DD_DJANGO_INCLUDE_USER_NAME", "origin": "default", "value": True},
+        {"name": "DD_DJANGO_INCLUDE_USER_REALNAME", "origin": "default", "value": False},
         {"name": "DD_DOGSTATSD_PORT", "origin": "default", "value": None},
         {"name": "DD_DOGSTATSD_URL", "origin": "default", "value": None},
         {"name": "DD_DYNAMIC_INSTRUMENTATION_DIAGNOSTICS_INTERVAL", "origin": "default", "value": 3600},