From fae48e61bccea15bfad184220a16b7302b6b3048 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20=C3=81lvarez=20=C3=81lvarez?= Date: Fri, 28 Feb 2025 14:13:28 +0100 Subject: [PATCH 1/2] Add setUser to user monitoring SDK --- .../AppSecEventTrackerSpecification.groovy | 33 ++++++++++++ ...ventTrackerAppSecDisabledForkedTest.groovy | 4 +- .../appsec/AppSecHttpServerTest.groovy | 4 +- .../SpringBootBasedTest.groovy | 54 ++++++++++++------- .../springsecurity5/UserController.groovy | 18 ++++++- dd-trace-api/build.gradle | 1 + .../java/datadog/appsec/api/user/User.java | 40 ++++++++++++++ .../datadog/appsec/api/user/UserService.java | 15 ++++++ .../trace/api/appsec/AppSecEventTracker.java | 37 ++++++++++++- 9 files changed, 179 insertions(+), 27 deletions(-) create mode 100644 dd-trace-api/src/main/java/datadog/appsec/api/user/User.java create mode 100644 dd-trace-api/src/main/java/datadog/appsec/api/user/UserService.java diff --git a/dd-java-agent/appsec/src/test/groovy/com/datadog/appsec/user/AppSecEventTrackerSpecification.groovy b/dd-java-agent/appsec/src/test/groovy/com/datadog/appsec/user/AppSecEventTrackerSpecification.groovy index 4721e6de178..cb392a582a4 100644 --- a/dd-java-agent/appsec/src/test/groovy/com/datadog/appsec/user/AppSecEventTrackerSpecification.groovy +++ b/dd-java-agent/appsec/src/test/groovy/com/datadog/appsec/user/AppSecEventTrackerSpecification.groovy @@ -3,6 +3,7 @@ package com.datadog.appsec.user import com.datadog.appsec.gateway.NoopFlow import datadog.appsec.api.blocking.BlockingContentType import datadog.appsec.api.blocking.BlockingException +import datadog.appsec.api.user.User import datadog.trace.api.EventTracker import datadog.trace.api.GlobalTracer import datadog.trace.api.ProductTraceSource @@ -31,6 +32,7 @@ import static datadog.trace.api.gateway.Events.EVENTS import static datadog.trace.api.telemetry.LoginEvent.LOGIN_FAILURE import static datadog.trace.api.telemetry.LoginEvent.LOGIN_SUCCESS import static datadog.trace.api.telemetry.LoginEvent.SIGN_UP +import static datadog.appsec.api.user.User.setUser class AppSecEventTrackerSpecification extends DDSpecification { @@ -72,6 +74,7 @@ class AppSecEventTrackerSpecification extends DDSpecification { } } GlobalTracer.setEventTracker(tracker) + User.setUserService(tracker) ActiveSubsystems.APPSEC_ACTIVE = true } @@ -128,6 +131,30 @@ class AppSecEventTrackerSpecification extends DDSpecification { 0 * _ } + def 'test track user (SDK)'() { + when: + setUser(USER_ID, ['key1': 'value1', 'key2': 'value2'], propagated) + + then: + 1 * traceSegment.setTagTop('usr.id', USER_ID) + 1 * traceSegment.setTagTop('usr', ['key1':'value1', 'key2':'value2']) + 1 * traceSegment.setTagTop('_dd.appsec.user.collection_mode', SDK.fullName()) + 1 * traceSegment.setTagTop('asm.keep', true) + 1 * traceSegment.setTagTop('_dd.p.ts', ProductTraceSource.ASM) + if (propagated) { + 1 * traceSegment.setTagTop('_dd.p.usr.id', USER_ID.bytes.encodeBase64().toString()) + } else { + 0 * traceSegment.setTagTop('_dd.p.usr.id', _) + } + 1 * user.apply(_ as RequestContext, USER_ID) >> NoopFlow.INSTANCE + 0 * _ + + where: + propagated | _ + true | _ + false | _ + } + def 'test wrong event argument validation (SDK)'() { when: GlobalTracer.getEventTracker().trackLoginSuccessEvent(null, null) @@ -164,6 +191,12 @@ class AppSecEventTrackerSpecification extends DDSpecification { then: thrown IllegalArgumentException + + when: + setUser(null, null) + + then: + thrown IllegalArgumentException } def "test onSignup (#mode)"() { diff --git a/dd-java-agent/appsec/src/test/groovy/com/datadog/appsec/user/EventTrackerAppSecDisabledForkedTest.groovy b/dd-java-agent/appsec/src/test/groovy/com/datadog/appsec/user/EventTrackerAppSecDisabledForkedTest.groovy index 3912a1785bc..9fd1ee9e10f 100644 --- a/dd-java-agent/appsec/src/test/groovy/com/datadog/appsec/user/EventTrackerAppSecDisabledForkedTest.groovy +++ b/dd-java-agent/appsec/src/test/groovy/com/datadog/appsec/user/EventTrackerAppSecDisabledForkedTest.groovy @@ -1,6 +1,6 @@ package com.datadog.appsec.user - +import datadog.appsec.api.user.User import datadog.trace.api.GlobalTracer import datadog.trace.api.UserIdCollectionMode import datadog.trace.api.appsec.AppSecEventTracker @@ -26,7 +26,7 @@ class EventTrackerAppSecDisabledForkedTest extends DDSpecification { void setup() { tracker = new AppSecEventTracker() GlobalTracer.setEventTracker(tracker) - + User.setUserService(tracker) traceSegment = Mock(TraceSegment) final tracer = Stub(AgentTracer.TracerAPI) { getTraceSegment() >> traceSegment diff --git a/dd-java-agent/appsec/src/testFixtures/groovy/com/datadog/appsec/AppSecHttpServerTest.groovy b/dd-java-agent/appsec/src/testFixtures/groovy/com/datadog/appsec/AppSecHttpServerTest.groovy index f03ed7b0a64..17105309c23 100644 --- a/dd-java-agent/appsec/src/testFixtures/groovy/com/datadog/appsec/AppSecHttpServerTest.groovy +++ b/dd-java-agent/appsec/src/testFixtures/groovy/com/datadog/appsec/AppSecHttpServerTest.groovy @@ -4,7 +4,6 @@ import datadog.communication.ddagent.SharedCommunicationObjects import datadog.communication.monitor.Monitoring import datadog.trace.agent.test.base.WithHttpServer import datadog.trace.api.Config -import datadog.trace.api.GlobalTracer import datadog.trace.api.appsec.AppSecEventTracker import datadog.trace.api.gateway.RequestContextSlot import datadog.trace.api.gateway.SubscriptionService @@ -26,8 +25,7 @@ abstract class AppSecHttpServerTest extends WithHttpServer { sco.createRemaining(config) assert sco.configurationPoller(config) == null assert sco.monitoring instanceof Monitoring.DisabledMonitoring - - GlobalTracer.setEventTracker(new AppSecEventTracker()) + AppSecEventTracker.install() AppSecSystem.start(ss, sco) } diff --git a/dd-java-agent/instrumentation/spring-security-5/src/test/groovy/datadog/trace/instrumentation/springsecurity5/SpringBootBasedTest.groovy b/dd-java-agent/instrumentation/spring-security-5/src/test/groovy/datadog/trace/instrumentation/springsecurity5/SpringBootBasedTest.groovy index cfbca2a4294..8e869deb22c 100644 --- a/dd-java-agent/instrumentation/spring-security-5/src/test/groovy/datadog/trace/instrumentation/springsecurity5/SpringBootBasedTest.groovy +++ b/dd-java-agent/instrumentation/spring-security-5/src/test/groovy/datadog/trace/instrumentation/springsecurity5/SpringBootBasedTest.groovy @@ -11,6 +11,7 @@ import datadog.trace.api.config.AppSecConfig import datadog.trace.core.DDSpan import okhttp3.FormBody import okhttp3.HttpUrl +import okhttp3.OkHttpClient import okhttp3.Request import okhttp3.RequestBody import org.springframework.boot.SpringApplication @@ -290,17 +291,7 @@ class SpringBootBasedTest extends AppSecHttpServerTest metadata) { + setUser(id, metadata, false); + } + + /** + * Sets the user monitoring tags on the root span using the prefix {@code usr} + * + * @param id identifier of the user + * @param metadata custom metadata data represented as key/value map + * @param propagated propagate the id to downstream services + */ + public static void setUser( + final String id, final Map metadata, final boolean propagated) { + SERVICE.trackUserEvent(id, metadata, propagated); + } +} diff --git a/dd-trace-api/src/main/java/datadog/appsec/api/user/UserService.java b/dd-trace-api/src/main/java/datadog/appsec/api/user/UserService.java new file mode 100644 index 00000000000..15a4cdf01b5 --- /dev/null +++ b/dd-trace-api/src/main/java/datadog/appsec/api/user/UserService.java @@ -0,0 +1,15 @@ +package datadog.appsec.api.user; + +import java.util.Map; + +public interface UserService { + + UserService NO_OP = + new UserService() { + @Override + public void trackUserEvent( + final String userId, final Map metadata, final boolean propagated) {} + }; + + void trackUserEvent(String userId, Map metadata, boolean propagated); +} diff --git a/internal-api/src/main/java/datadog/trace/api/appsec/AppSecEventTracker.java b/internal-api/src/main/java/datadog/trace/api/appsec/AppSecEventTracker.java index 2fed75c22cb..a214aa5c0f6 100644 --- a/internal-api/src/main/java/datadog/trace/api/appsec/AppSecEventTracker.java +++ b/internal-api/src/main/java/datadog/trace/api/appsec/AppSecEventTracker.java @@ -8,8 +8,11 @@ import static datadog.trace.api.telemetry.LoginEvent.LOGIN_SUCCESS; import static datadog.trace.api.telemetry.LoginEvent.SIGN_UP; import static datadog.trace.util.Strings.toHexString; +import static java.util.Collections.emptyMap; import datadog.appsec.api.blocking.BlockingException; +import datadog.appsec.api.user.User; +import datadog.appsec.api.user.UserService; import datadog.trace.api.EventTracker; import datadog.trace.api.GlobalTracer; import datadog.trace.api.ProductTraceSource; @@ -27,10 +30,11 @@ import datadog.trace.bootstrap.instrumentation.api.Tags; import java.security.MessageDigest; import java.security.NoSuchAlgorithmException; +import java.util.Base64; import java.util.Map; import java.util.function.BiFunction; -public class AppSecEventTracker extends EventTracker { +public class AppSecEventTracker extends EventTracker implements UserService { private static final int HASH_SIZE_BYTES = 16; // 128 bits private static final String ANON_PREFIX = "anon_"; @@ -40,7 +44,9 @@ public class AppSecEventTracker extends EventTracker { private static final String SIGNUP_TAG = "users.signup"; public static void install() { - GlobalTracer.setEventTracker(new AppSecEventTracker()); + final AppSecEventTracker tracker = new AppSecEventTracker(); + GlobalTracer.setEventTracker(tracker); + User.setUserService(tracker); } @Override @@ -68,6 +74,15 @@ public final void trackCustomEvent(String eventName, Map metadat onCustomEvent(SDK, eventName, metadata); } + @Override + public void trackUserEvent( + final String userId, final Map metadata, final boolean propagated) { + if (userId == null || userId.isEmpty()) { + throw new IllegalArgumentException("UserId is null or empty"); + } + onUserEvent(SDK, userId, metadata, propagated); + } + public void onUserNotFound(final UserIdCollectionMode mode) { if (!isEnabled(mode)) { return; @@ -88,6 +103,14 @@ public void onUserNotFound(final UserIdCollectionMode mode) { } public void onUserEvent(final UserIdCollectionMode mode, final String userId) { + onUserEvent(mode, userId, emptyMap(), false); + } + + public void onUserEvent( + final UserIdCollectionMode mode, + final String userId, + final Map metadata, + final boolean propagated) { if (!isEnabled(mode)) { return; } @@ -105,9 +128,15 @@ public void onUserEvent(final UserIdCollectionMode mode, final String userId) { } if (mode != SDK) { segment.setTagTop("_dd.appsec.usr.id", finalUserId); + } else if (propagated) { + // only the SDK can propagate usr.id + segment.setTagTop("_dd.p.usr.id", encodeBase64(finalUserId)); } if (isNewUser(mode, segment)) { segment.setTagTop("usr.id", finalUserId); + if (metadata != null && !metadata.isEmpty()) { + segment.setTagTop("usr", metadata); + } segment.setTagTop("_dd.appsec.user.collection_mode", mode.fullName()); segment.setTagTop(Tags.ASM_KEEP, true); segment.setTagTop(Tags.PROPAGATED_TRACE_SOURCE, ProductTraceSource.ASM); @@ -357,6 +386,10 @@ protected static String anonymizeUser(final UserIdCollectionMode mode, final Str return ANON_PREFIX + toHexString(hash); } + protected static String encodeBase64(final String userId) { + return Base64.getEncoder().encodeToString(userId.getBytes()); + } + protected boolean isEnabled(final UserIdCollectionMode mode) { return mode == SDK || (ActiveSubsystems.APPSEC_ACTIVE && mode != DISABLED); } From d51ce62aae41d7bfa49022d09afcb2e1d462be0f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20=C3=81lvarez=20=C3=81lvarez?= Date: Tue, 18 Mar 2025 09:41:18 +0100 Subject: [PATCH 2/2] Remove user identity propagation --- .../user/AppSecEventTrackerSpecification.groovy | 12 +----------- .../main/java/datadog/appsec/api/user/User.java | 14 +------------- .../java/datadog/appsec/api/user/UserService.java | 5 ++--- .../trace/api/appsec/AppSecEventTracker.java | 15 ++++----------- 4 files changed, 8 insertions(+), 38 deletions(-) diff --git a/dd-java-agent/appsec/src/test/groovy/com/datadog/appsec/user/AppSecEventTrackerSpecification.groovy b/dd-java-agent/appsec/src/test/groovy/com/datadog/appsec/user/AppSecEventTrackerSpecification.groovy index cb392a582a4..79021938cb5 100644 --- a/dd-java-agent/appsec/src/test/groovy/com/datadog/appsec/user/AppSecEventTrackerSpecification.groovy +++ b/dd-java-agent/appsec/src/test/groovy/com/datadog/appsec/user/AppSecEventTrackerSpecification.groovy @@ -133,7 +133,7 @@ class AppSecEventTrackerSpecification extends DDSpecification { def 'test track user (SDK)'() { when: - setUser(USER_ID, ['key1': 'value1', 'key2': 'value2'], propagated) + setUser(USER_ID, ['key1': 'value1', 'key2': 'value2']) then: 1 * traceSegment.setTagTop('usr.id', USER_ID) @@ -141,18 +141,8 @@ class AppSecEventTrackerSpecification extends DDSpecification { 1 * traceSegment.setTagTop('_dd.appsec.user.collection_mode', SDK.fullName()) 1 * traceSegment.setTagTop('asm.keep', true) 1 * traceSegment.setTagTop('_dd.p.ts', ProductTraceSource.ASM) - if (propagated) { - 1 * traceSegment.setTagTop('_dd.p.usr.id', USER_ID.bytes.encodeBase64().toString()) - } else { - 0 * traceSegment.setTagTop('_dd.p.usr.id', _) - } 1 * user.apply(_ as RequestContext, USER_ID) >> NoopFlow.INSTANCE 0 * _ - - where: - propagated | _ - true | _ - false | _ } def 'test wrong event argument validation (SDK)'() { diff --git a/dd-trace-api/src/main/java/datadog/appsec/api/user/User.java b/dd-trace-api/src/main/java/datadog/appsec/api/user/User.java index d8a94bd813d..269d2e45d97 100644 --- a/dd-trace-api/src/main/java/datadog/appsec/api/user/User.java +++ b/dd-trace-api/src/main/java/datadog/appsec/api/user/User.java @@ -23,18 +23,6 @@ public static void setUserService(final UserService service) { * @param metadata custom metadata data represented as key/value map */ public static void setUser(final String id, final Map metadata) { - setUser(id, metadata, false); - } - - /** - * Sets the user monitoring tags on the root span using the prefix {@code usr} - * - * @param id identifier of the user - * @param metadata custom metadata data represented as key/value map - * @param propagated propagate the id to downstream services - */ - public static void setUser( - final String id, final Map metadata, final boolean propagated) { - SERVICE.trackUserEvent(id, metadata, propagated); + SERVICE.trackUserEvent(id, metadata); } } diff --git a/dd-trace-api/src/main/java/datadog/appsec/api/user/UserService.java b/dd-trace-api/src/main/java/datadog/appsec/api/user/UserService.java index 15a4cdf01b5..da95a76bc34 100644 --- a/dd-trace-api/src/main/java/datadog/appsec/api/user/UserService.java +++ b/dd-trace-api/src/main/java/datadog/appsec/api/user/UserService.java @@ -7,9 +7,8 @@ public interface UserService { UserService NO_OP = new UserService() { @Override - public void trackUserEvent( - final String userId, final Map metadata, final boolean propagated) {} + public void trackUserEvent(final String userId, final Map metadata) {} }; - void trackUserEvent(String userId, Map metadata, boolean propagated); + void trackUserEvent(String userId, Map metadata); } diff --git a/internal-api/src/main/java/datadog/trace/api/appsec/AppSecEventTracker.java b/internal-api/src/main/java/datadog/trace/api/appsec/AppSecEventTracker.java index a214aa5c0f6..8f969503d43 100644 --- a/internal-api/src/main/java/datadog/trace/api/appsec/AppSecEventTracker.java +++ b/internal-api/src/main/java/datadog/trace/api/appsec/AppSecEventTracker.java @@ -75,12 +75,11 @@ public final void trackCustomEvent(String eventName, Map metadat } @Override - public void trackUserEvent( - final String userId, final Map metadata, final boolean propagated) { + public void trackUserEvent(final String userId, final Map metadata) { if (userId == null || userId.isEmpty()) { throw new IllegalArgumentException("UserId is null or empty"); } - onUserEvent(SDK, userId, metadata, propagated); + onUserEvent(SDK, userId, metadata); } public void onUserNotFound(final UserIdCollectionMode mode) { @@ -103,14 +102,11 @@ public void onUserNotFound(final UserIdCollectionMode mode) { } public void onUserEvent(final UserIdCollectionMode mode, final String userId) { - onUserEvent(mode, userId, emptyMap(), false); + onUserEvent(mode, userId, emptyMap()); } public void onUserEvent( - final UserIdCollectionMode mode, - final String userId, - final Map metadata, - final boolean propagated) { + final UserIdCollectionMode mode, final String userId, final Map metadata) { if (!isEnabled(mode)) { return; } @@ -128,9 +124,6 @@ public void onUserEvent( } if (mode != SDK) { segment.setTagTop("_dd.appsec.usr.id", finalUserId); - } else if (propagated) { - // only the SDK can propagate usr.id - segment.setTagTop("_dd.p.usr.id", encodeBase64(finalUserId)); } if (isNewUser(mode, segment)) { segment.setTagTop("usr.id", finalUserId);