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.

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

Common hook/listener for authentication results for inbound requests #14046

Closed
philsttr opened this issue Oct 23, 2023 · 8 comments
Closed

Common hook/listener for authentication results for inbound requests #14046

philsttr opened this issue Oct 23, 2023 · 8 comments
Assignees
Labels
type: enhancement A general enhancement

Comments

@philsttr
Copy link
Contributor

I've been looking into adding instrumentation to OpenTelemetry to capture the enduser.id, enduser.role, and enduser.scope attributes as part of Server spans for applications using Spring Security. In Spring Security terms, this means retrieving the Authentication principal name, roles, and scopes from the Authentication object obtained for a successful authentication for inbound requests. I'm trying to identify the best location in Spring Security to instrument to capture those attributes from an Authentication object.

I have found is that there is no "single place" to capture the Authentication object that results from successful authentications for inbound requests.

I considered instrumenting SecurityContext or ReactiveSecurityContext to capture the Authentication from when the SecurityContext or ReactiveSecurityContext is written. But, unfortunately, they can be written in places that aren't just for inbound requests. For example, it could be for async processing, or scheduled tasks, or the application could even switch the active authentication during request processing for impersonation. Therefore, this would catch more use cases than desired.

Next, I looked at all the places that perform authentication for inbound requests. Some examples:

Reactive:

Servlet:

So, one option would be to add OpenTelemetry instrumentation to each of these places. While this is possible, it's not ideal because it is very fragile. For example, new filters that authenticate inbound requests won't be covered, and any changes to any of those locations could also break the instrumentation. It also requires different strategies in each case for actually capturing the Authentication object, since not every location encapsulates the Authentication object in the same way.

What I would really love is some kind of hook or listener that the instrumentation could use to listen for successful authentications for inbound requests. Perhaps these locations could fire off a Spring event, and the instrumentation could listen for those events (assuming the callback runs on the same thread, so the instrumentation has access to the active Span).

Would it be possible for spring-security to add a hook/listener that is triggered with authentication attempt results for inbound requests? This would make writing this type of automatic instrumentation easier.

Would you recommend a different approach?

@marcusdacoregio
Copy link
Contributor

Hi, @philsttr.

I believe you would be able to do it by relying on the SecurityContextChangedEventListener. Although that listener will be triggered for any change in the context, not only for HTTP requests, you can use org.springframework.web.context.request.RequestContextHolder#getRequestAttributes to check if there is a RequestAttributes bound to the thread. It would look similar to this:

@Bean
static SecurityContextHolderStrategy securityContextHolderStrategy() {
	SecurityContextHolderStrategy original = SecurityContextHolder.getContextHolderStrategy();
	SecurityContextChangedListener listener = new MyListener();
	SecurityContextHolderStrategy strategy = new ListeningSecurityContextHolderStrategy(original, listener);
	SecurityContextHolder.setContextHolderStrategy(strategy);
	return strategy;
}

static class MyListener implements SecurityContextChangedListener {

	@Override
	public void securityContextChanged(SecurityContextChangedEvent event) {
		RequestAttributes requestAttributes = RequestContextHolder.getRequestAttributes();
		if (requestAttributes != null) {
			// there is a request bound to the thread
                         // you can even check if requestAttributes instanceof ServletRequestAttributes
		}
	}

}

@jzheaux do you have any other suggestions on how to achieve this?

@philsttr
Copy link
Contributor Author

Thanks for the response @marcusdacoregio.

A couple problems with that approach:

  1. RequestContextHolder only works in servlet based apps, and does not work in webflux apps. Is there an equivalent for webflux apps? The listener would also need access to the reactor subscriber context, since that is where the OpenTelemetry span information would reside for webflux apps. (as opposed to a ThreadLocal)
  2. It could potentially trigger a second time if an app switched the context after the security filters initially set the context. Although, this situation might be able to be detected by checking to see if securityContextChangedEvent.getOldContext() does not contain an Authentication. Would that work?

@marcusdacoregio
Copy link
Contributor

In Spring Security terms, this means retrieving the Authentication principal name, roles, and scopes from the Authentication object obtained for a successful authentication for inbound requests

What do you mean by successful authentication? Do you want to capture those attributes only when the user performs the authentication? Or, if the session contains an authenticated security context, would you consider that too? Have you considered implementing a filter that captures the attributes for each request?

@philsttr
Copy link
Contributor Author

philsttr commented Oct 24, 2023

Excellent questions. I'll clarify.

I want to capture the attributes for each inbound request that has an associated Authentication (either because authentication was performed earlier in the request processing, or the request is part of an authenticated session).

I have not considered implementing a filter, because I'm not quite sure how the OpenTelemetry Java Agent instrumentation could inject that filter into the "right" spot in the filter chain for all (or at least the vast majority of) apps. The agent is intended to "just work" when enabled for an application, so it is important that it works in a wide variety of apps. The agent can easily inject bytecode at strategic spots that would work across all/majority of apps. It seemed easier to me for the agent to inject some bytecode, rather than manipulate one or more filter chains in an app.

@philsttr
Copy link
Contributor Author

You got me thinking about filters.

Perhaps the agent could instrument the build() methods of ServerHttpSecurity and HttpSecurity to add a filter before SecurityWebFiltersOrder.LOGOUT. Then the filter could capture the attributes from the current Authentication. Seems pretty straightforward.

@marcusdacoregio
Copy link
Contributor

Sounds like a good place to do it. I think that for servlet apps you should place your filter before the AuthorizationFilter since logout is pretty early in the filter chain. Note that the SwitchUserFilter is placed after the AuthorizationFilter.

@philsttr
Copy link
Contributor Author

philsttr commented Oct 25, 2023

Got it, yeah I was looking at the reactive filter chain. I assumed the servlet was similar, but it's a bit different as you mentioned.

I have a working implementation of the reactive instrumentation (will be contributing it as part of open-telemetry/opentelemetry-java-instrumentation#9400). It's pretty clean, and I'm pretty happy with it. So I'm going to close this issue.

Thanks for the brainstorming and the help!

@philsttr
Copy link
Contributor Author

philsttr commented Nov 6, 2023

For reference: open-telemetry/opentelemetry-java-instrumentation#9777 adds the OpenTelemetry instrumentation described above

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

2 participants