Skip to content

Commit a219d38

Browse files
manuel-alvarez-alvarezmanuel-alvarez-alvarez
committed
Prevent spans from having login success and failure events at the same time
1 parent 39c11f9 commit a219d38

File tree

9 files changed

+184
-44
lines changed

9 files changed

+184
-44
lines changed

‎dd-java-agent/instrumentation/spring-security-5/src/main/java/datadog/trace/instrumentation/springsecurity5/AbstractAuthenticationProcessingFilterInstrumentation.java‎

Lines changed: 70 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,70 @@
1+
package datadog.trace.instrumentation.springsecurity5;
2+
3+
import static datadog.trace.agent.tooling.bytebuddy.matcher.NameMatchers.named;
4+
import static net.bytebuddy.matcher.ElementMatchers.isMethod;
5+
import static net.bytebuddy.matcher.ElementMatchers.takesArgument;
6+
7+
import com.google.auto.service.AutoService;
8+
import datadog.trace.agent.tooling.Instrumenter;
9+
import datadog.trace.agent.tooling.InstrumenterModule;
10+
import datadog.trace.bootstrap.ActiveSubsystems;
11+
import net.bytebuddy.asm.Advice;
12+
import org.springframework.security.core.Authentication;
13+
import org.springframework.security.core.AuthenticationException;
14+
15+
@AutoService(InstrumenterModule.class)
16+
public class AbstractAuthenticationProcessingFilterInstrumentation extends InstrumenterModule.AppSec
17+
implements Instrumenter.ForSingleType {
18+
19+
public AbstractAuthenticationProcessingFilterInstrumentation() {
20+
super("spring-security");
21+
}
22+
23+
@Override
24+
public String instrumentedType() {
25+
return "org.springframework.security.web.authentication.AbstractAuthenticationProcessingFilter";
26+
}
27+
28+
@Override
29+
public String[] helperClassNames() {
30+
return new String[] {
31+
"datadog.trace.instrumentation.springsecurity5.SpringSecurityUserEventDecorator"
32+
};
33+
}
34+
35+
@Override
36+
public void methodAdvice(MethodTransformer transformer) {
37+
transformer.applyAdvice(
38+
isMethod()
39+
.and(named("successfulAuthentication"))
40+
.and(takesArgument(3, named("org.springframework.security.core.Authentication"))),
41+
getClass().getName() + "$SuccessfulAuthenticationAdvice");
42+
transformer.applyAdvice(
43+
isMethod()
44+
.and(named("unsuccessfulAuthentication"))
45+
.and(
46+
takesArgument(
47+
2, named("org.springframework.security.core.AuthenticationException"))),
48+
getClass().getName() + "$UnsuccessfulAuthenticationAdvice");
49+
}
50+
51+
public static class SuccessfulAuthenticationAdvice {
52+
53+
@Advice.OnMethodExit(suppress = Throwable.class)
54+
public static void onExit(@Advice.Argument(3) Authentication authentication) {
55+
if (ActiveSubsystems.APPSEC_ACTIVE) {
56+
SpringSecurityUserEventDecorator.DECORATE.onLoginSuccess(authentication);
57+
}
58+
}
59+
}
60+
61+
public static class UnsuccessfulAuthenticationAdvice {
62+
63+
@Advice.OnMethodExit(suppress = Throwable.class)
64+
public static void onExit(@Advice.Argument(2) AuthenticationException exception) {
65+
if (ActiveSubsystems.APPSEC_ACTIVE) {
66+
SpringSecurityUserEventDecorator.DECORATE.onLoginFailure(exception);
67+
}
68+
}
69+
}
70+
}

dd-java-agent/instrumentation/spring-security-5/src/main/java/datadog/trace/instrumentation/springsecurity5/AuthenticationProviderInstrumentation.java renamed to dd-java-agent/instrumentation/spring-security-5/src/main/java/datadog/trace/instrumentation/springsecurity5/AuthenticationManagerInstrumentation.java

Lines changed: 21 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -11,23 +11,24 @@
1111
import datadog.trace.agent.tooling.Instrumenter;
1212
import datadog.trace.agent.tooling.InstrumenterModule;
1313
import datadog.trace.bootstrap.ActiveSubsystems;
14+
import datadog.trace.bootstrap.CallDepthThreadLocalMap;
1415
import net.bytebuddy.asm.Advice;
1516
import net.bytebuddy.description.type.TypeDescription;
1617
import net.bytebuddy.matcher.ElementMatcher;
18+
import org.springframework.security.authentication.AuthenticationManager;
1719
import org.springframework.security.core.Authentication;
18-
import org.springframework.security.core.AuthenticationException;
1920

2021
@AutoService(InstrumenterModule.class)
21-
public class AuthenticationProviderInstrumentation extends InstrumenterModule.AppSec
22+
public class AuthenticationManagerInstrumentation extends InstrumenterModule.AppSec
2223
implements Instrumenter.ForTypeHierarchy {
2324

24-
public AuthenticationProviderInstrumentation() {
25+
public AuthenticationManagerInstrumentation() {
2526
super("spring-security");
2627
}
2728

2829
@Override
2930
public String hierarchyMarkerType() {
30-
return "org.springframework.security.authentication.AuthenticationProvider";
31+
return "org.springframework.security.authentication.AuthenticationManager";
3132
}
3233

3334
@Override
@@ -50,19 +51,26 @@ public void methodAdvice(MethodTransformer transformer) {
5051
.and(takesArgument(0, named("org.springframework.security.core.Authentication")))
5152
.and(returns(named("org.springframework.security.core.Authentication")))
5253
.and(isPublic()),
53-
getClass().getName() + "$AuthenticationProviderAdvice");
54+
getClass().getName() + "$AuthenticationManagerAdvice");
5455
}
5556

56-
public static class AuthenticationProviderAdvice {
57+
public static class AuthenticationManagerAdvice {
5758

58-
@Advice.OnMethodExit(onThrowable = AuthenticationException.class, suppress = Throwable.class)
59-
public static void onExit(
60-
@Advice.Argument(value = 0, readOnly = false) Authentication authentication,
61-
@Advice.Return final Authentication result,
62-
@Advice.Thrown final AuthenticationException throwable) {
63-
if (ActiveSubsystems.APPSEC_ACTIVE) {
64-
SpringSecurityUserEventDecorator.DECORATE.onLogin(authentication, throwable, result);
59+
@Advice.OnMethodEnter(suppress = Throwable.class)
60+
public static int onEnter(@Advice.Argument(0) Authentication authentication) {
61+
int depth = CallDepthThreadLocalMap.incrementCallDepth(AuthenticationManager.class);
62+
if (depth == 0 && ActiveSubsystems.APPSEC_ACTIVE) {
63+
SpringSecurityUserEventDecorator.DECORATE.onAuthentication(authentication);
6564
}
65+
return depth;
66+
}
67+
68+
@Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class)
69+
public static void onExit(@Advice.Enter final int depth) {
70+
if (depth > 0) {
71+
return;
72+
}
73+
CallDepthThreadLocalMap.reset(AuthenticationManager.class);
6674
}
6775
}
6876
}

‎dd-java-agent/instrumentation/spring-security-5/src/main/java/datadog/trace/instrumentation/springsecurity5/SpringSecurityUserEventDecorator.java‎

Lines changed: 44 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515
import org.slf4j.LoggerFactory;
1616
import org.springframework.security.authentication.UsernamePasswordAuthenticationToken;
1717
import org.springframework.security.core.Authentication;
18+
import org.springframework.security.core.AuthenticationException;
1819
import org.springframework.security.core.userdetails.User;
1920
import org.springframework.security.core.userdetails.UserDetails;
2021

@@ -30,6 +31,12 @@ public class SpringSecurityUserEventDecorator {
3031
private static final Set<Class<?>> SKIPPED_AUTHS =
3132
Collections.newSetFromMap(new ConcurrentHashMap<>());
3233

34+
private static final ThreadLocal<Authentication> AUTHENTICATION = new ThreadLocal<>();
35+
36+
public void onAuthentication(final Authentication authentication) {
37+
AUTHENTICATION.set(authentication);
38+
}
39+
3340
public void onUserNotFound() {
3441
final AppSecEventTracker tracker = AppSecEventTracker.getEventTracker();
3542
if (tracker == null) {
@@ -65,11 +72,40 @@ public void onSignup(UserDetails user, Throwable throwable) {
6572
tracker.onSignupEvent(mode, userId, metadata);
6673
}
6774

68-
public void onLogin(Authentication authentication, Throwable throwable, Authentication result) {
69-
if (authentication == null) {
75+
public void onLoginSuccess(Authentication authentication) {
76+
AUTHENTICATION.remove();
77+
78+
final AppSecEventTracker tracker = AppSecEventTracker.getEventTracker();
79+
if (tracker == null) {
7080
return;
7181
}
7282

83+
if (shouldSkipAuthentication(authentication)) {
84+
return;
85+
}
86+
87+
UserIdCollectionMode mode = UserIdCollectionMode.get();
88+
String userId = authentication.getName();
89+
Map<String, String> metadata = null;
90+
Object principal = authentication.getPrincipal();
91+
if (principal instanceof User) {
92+
User user = (User) principal;
93+
metadata = new HashMap<>();
94+
metadata.put("enabled", String.valueOf(user.isEnabled()));
95+
metadata.put(
96+
"authorities",
97+
user.getAuthorities().stream().map(Object::toString).collect(Collectors.joining(",")));
98+
metadata.put("accountNonExpired", String.valueOf(user.isAccountNonExpired()));
99+
metadata.put("accountNonLocked", String.valueOf(user.isAccountNonLocked()));
100+
metadata.put("credentialsNonExpired", String.valueOf(user.isCredentialsNonExpired()));
101+
}
102+
tracker.onLoginSuccessEvent(mode, userId, metadata);
103+
}
104+
105+
public void onLoginFailure(AuthenticationException exception) {
106+
Authentication authentication = AUTHENTICATION.get();
107+
AUTHENTICATION.remove();
108+
73109
final AppSecEventTracker tracker = AppSecEventTracker.getEventTracker();
74110
if (tracker == null) {
75111
return;
@@ -80,35 +116,9 @@ public void onLogin(Authentication authentication, Throwable throwable, Authenti
80116
}
81117

82118
UserIdCollectionMode mode = UserIdCollectionMode.get();
83-
String userId = result != null ? result.getName() : authentication.getName();
84-
if (throwable == null && result != null && result.isAuthenticated()) {
85-
Map<String, String> metadata = null;
86-
Object principal = result.getPrincipal();
87-
if (principal instanceof User) {
88-
User user = (User) principal;
89-
metadata = new HashMap<>();
90-
metadata.put("enabled", String.valueOf(user.isEnabled()));
91-
metadata.put(
92-
"authorities",
93-
user.getAuthorities().stream().map(Object::toString).collect(Collectors.joining(",")));
94-
metadata.put("accountNonExpired", String.valueOf(user.isAccountNonExpired()));
95-
metadata.put("accountNonLocked", String.valueOf(user.isAccountNonLocked()));
96-
metadata.put("credentialsNonExpired", String.valueOf(user.isCredentialsNonExpired()));
97-
}
98-
tracker.onLoginSuccessEvent(mode, userId, metadata);
99-
} else if (throwable != null) {
100-
// On bad password, throwable would be
101-
// org.springframework.security.authentication.BadCredentialsException,
102-
// on user not found, throwable can be BadCredentials or
103-
// org.springframework.security.core.userdetails.UsernameNotFoundException depending on the
104-
// internal setting
105-
// hideUserNotFoundExceptions (or a custom AuthenticationProvider implementation overriding
106-
// this).
107-
// This would be the ideal place to check whether the user exists or not, but we cannot do
108-
// so reliably yet.
109-
// See UsernameNotFoundExceptionInstrumentation for more details.
110-
tracker.onLoginFailureEvent(mode, userId, null, null);
111-
}
119+
String userId = authentication.getName();
120+
121+
tracker.onLoginFailureEvent(mode, userId, null, null);
112122
}
113123

114124
public void onUser(final Authentication authentication) {
@@ -131,6 +141,9 @@ public void onUser(final Authentication authentication) {
131141
}
132142

133143
private static boolean shouldSkipAuthentication(final Authentication authentication) {
144+
if (authentication == null) {
145+
return true;
146+
}
134147
if (authentication instanceof UsernamePasswordAuthenticationToken) {
135148
return false;
136149
}

‎dd-java-agent/instrumentation/spring-security-5/src/test/groovy/datadog/trace/instrumentation/springsecurity5/SecurityConfig.groovy‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,9 +2,13 @@ package datadog.trace.instrumentation.springsecurity5
22

33
import custom.CustomAuthenticationFilter
44
import custom.CustomAuthenticationProvider
5+
import custom.FailingAuthenticationProvider
56
import org.springframework.context.annotation.Bean
67
import org.springframework.context.annotation.Configuration
78
import org.springframework.security.authentication.AuthenticationManager
9+
import org.springframework.security.authentication.AuthenticationProvider
10+
import org.springframework.security.authentication.ProviderManager
11+
import org.springframework.security.config.annotation.ObjectPostProcessor
812
import org.springframework.security.config.annotation.web.builders.HttpSecurity
913
import org.springframework.security.config.annotation.web.configuration.EnableWebSecurity
1014
import org.springframework.security.config.annotation.web.configurers.AbstractHttpConfigurer
@@ -49,6 +53,7 @@ class SecurityConfig {
4953
@Override
5054
void configure(HttpSecurity http) throws Exception {
5155
AuthenticationManager authenticationManager = http.getSharedObject(AuthenticationManager)
56+
http.authenticationProvider(new FailingAuthenticationProvider())
5257
http.authenticationProvider(new CustomAuthenticationProvider())
5358
http.addFilterBefore(new CustomAuthenticationFilter(authenticationManager), UsernamePasswordAuthenticationFilter)
5459
}

‎dd-java-agent/instrumentation/spring-security-5/src/test/groovy/datadog/trace/instrumentation/springsecurity5/SpringBootBasedTest.groovy‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -235,6 +235,8 @@ class SpringBootBasedTest extends AppSecHttpServerTest<ConfigurableApplicationCo
235235
span.getTag("appsec.events.users.login.success")['enabled'] == 'true'
236236
span.getTag("appsec.events.users.login.success")['authorities'] == 'ROLE_USER'
237237
span.getTag("appsec.events.users.login.success")['accountNonLocked'] == 'true'
238+
239+
span.getTag("appsec.events.users.login.failure.track") == null
238240
}
239241

240242
void 'test failed signup'() {

‎dd-java-agent/instrumentation/spring-security-5/src/test/java/custom/FailingAuthenticationProvider.java‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
package custom;
2+
3+
import org.springframework.security.authentication.AuthenticationProvider;
4+
import org.springframework.security.authentication.AuthenticationServiceException;
5+
import org.springframework.security.core.Authentication;
6+
import org.springframework.security.core.AuthenticationException;
7+
8+
public class FailingAuthenticationProvider implements AuthenticationProvider {
9+
10+
@Override
11+
public Authentication authenticate(Authentication authentication) throws AuthenticationException {
12+
throw new AuthenticationServiceException("I'm dumb");
13+
}
14+
15+
@Override
16+
public boolean supports(Class<?> authentication) {
17+
return true;
18+
}
19+
}

‎dd-java-agent/instrumentation/spring-security-6/src/test/groovy/datadog/trace/instrumentation/springsecurity6/SecurityConfig.groovy‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ package datadog.trace.instrumentation.springsecurity6
22

33
import custom.CustomAuthenticationFilter
44
import custom.CustomAuthenticationProvider
5+
import custom.FailingAuthenticationProvider
56
import org.springframework.context.annotation.Bean
67
import org.springframework.context.annotation.Configuration
78
import org.springframework.security.authentication.AuthenticationManager
@@ -47,6 +48,7 @@ class SecurityConfig {
4748
@Override
4849
void configure(HttpSecurity http) throws Exception {
4950
AuthenticationManager authenticationManager = http.getSharedObject(AuthenticationManager)
51+
http.authenticationProvider(new FailingAuthenticationProvider())
5052
http.authenticationProvider(new CustomAuthenticationProvider())
5153
http.addFilterBefore(new CustomAuthenticationFilter(authenticationManager), UsernamePasswordAuthenticationFilter)
5254
}

‎dd-java-agent/instrumentation/spring-security-6/src/test/groovy/datadog/trace/instrumentation/springsecurity6/SpringBootBasedTest.groovy‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -211,6 +211,8 @@ class SpringBootBasedTest extends AppSecHttpServerTest<ConfigurableApplicationCo
211211
span.getTag("appsec.events.users.login.success")['enabled'] == 'true'
212212
span.getTag("appsec.events.users.login.success")['authorities'] == 'ROLE_USER'
213213
span.getTag("appsec.events.users.login.success")['accountNonLocked'] == 'true'
214+
215+
span.getTag("appsec.events.users.login.failure.track") == null
214216
}
215217

216218
void 'test failed signup'() {

‎dd-java-agent/instrumentation/spring-security-6/src/test/java/custom/FailingAuthenticationProvider.java‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
package custom;
2+
3+
import org.springframework.security.authentication.AuthenticationProvider;
4+
import org.springframework.security.authentication.AuthenticationServiceException;
5+
import org.springframework.security.core.Authentication;
6+
import org.springframework.security.core.AuthenticationException;
7+
8+
public class FailingAuthenticationProvider implements AuthenticationProvider {
9+
10+
@Override
11+
public Authentication authenticate(Authentication authentication) throws AuthenticationException {
12+
throw new AuthenticationServiceException("I'm dumb");
13+
}
14+
15+
@Override
16+
public boolean supports(Class<?> authentication) {
17+
return true;
18+
}
19+
}

0 commit comments

Comments
 (0)