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

Capture identity attributes (enduser.*) for applications using Spring Security #9400

Open
philsttr opened this issue Sep 6, 2023 · 11 comments
Labels
contribution welcome Request makes sense, maintainers probably won't have time, contribution would be welcome enhancement New feature or request new instrumentation

Comments

@philsttr
Copy link
Contributor

philsttr commented Sep 6, 2023

Is your feature request related to a problem? Please describe.

General identity attributes (enduser.id, enduser.role, enduser.scope) are not captured for SERVER spans in applications using Spring Security OAuth2 Resource Server for Servlet or WebFlux based applications.

The existing servlet instrumentation currently captures the enduser.id in a couple places (here and here). However, in an Servlet-based application using Spring Security OAuth2 Resource Server, the request.getUserPrincipal() returns null at this level. It returns null here because Spring Security wraps that request object in a request wrapper at a higher level, and the user principal is only returned from the wrapper at the higher level. In other words, since the servlet instrumentation only has access to the lower level request, and not the higher level request wrapper, the user principal seen by the servlet instrumentation is null, and therefore the enduser.id is not captured.

In addition, there is no existing instrumentation for capturing the enduser.id in WebFlux applications.

Describe the solution you'd like

I would like general identity attributes (enduser.id, enduser.role, enduser.scope) to be captured for SERVER spans in applications using Spring Security OAuth2 Resource Server for Servlet and WebFlux based applications.

Describe alternatives you've considered

No response

Additional context

While the new instrumentation probably needs to be Spring Security specific, it does not necessarily need to be specific to OAuth2 Resource Servers authentication/authorization. i.e. The new instrumentation could probably work for any type of Spring Security based authentication/authorization.

@philsttr philsttr added enhancement New feature or request needs triage New issue that requires triage labels Sep 6, 2023
@mateuszrzeszutek
Copy link
Member

Hey @philsttr ,

I think this'd be a welcome addition 👍
We'd have to make the new instrumentation disabled by default because of the

Given the sensitive nature of this information, SDKs and exporters SHOULD drop these attributes by default and then provide a configuration parameter to turn on retention for use cases where the information is required and would not violate any policies or regulations.

fragment in the spec theough.

@mateuszrzeszutek mateuszrzeszutek added contribution welcome Request makes sense, maintainers probably won't have time, contribution would be welcome new instrumentation and removed needs triage New issue that requires triage labels Sep 6, 2023
@philsttr
Copy link
Contributor Author

philsttr commented Sep 6, 2023

Agreed. Probably should do that for the existing servlet instrumentation that sets enduser.id as well, but that's probably another issue.

@swar8080
Copy link
Contributor

swar8080 commented Sep 9, 2023

Maybe these attributes could be captured for most spring security use cases by instrumenting SecurityContextHolder#setContext? From there a list of GrantedAuthority are accessible, which is where roles and scopes are stored. The spring convention is that roles start with the prefix ROLE_, and the oauth2 resource server library stores scopes as granted authorities prefixed with SCOPE_ by default

One of the things i'm not sure about is this part of the convention:

It is expected this information would be propagated unchanged from node-to-node within the system using the Baggage mechanism. These attributes should not be used to record system-to-system authentication attributes.

For example, i've seen production queue consumer code that manually calls SecurityContextHolder#setContext. Including these attributes as baggage on HTTP requests by that queue consumer could be unexpected. So maybe more focused instrumentation is needed to avoid that, like on the specific spring security oauth2 resource server http filters.

@philsttr
Copy link
Contributor Author

Filed #9740 for updating the existing servlet instrumentation to not capture enduser.id by default.

@philsttr
Copy link
Contributor Author

philsttr commented Oct 23, 2023

Looking for advice on how to best implement this requirement..

Given the sensitive nature of this information, SDKs and exporters SHOULD drop these attributes by default and then provide a configuration parameter to turn on retention for use cases where the information is required and would not violate any policies or regulations.

Should this requirement be implemented by:

  1. having each instrumentation that captures the endpoint.* attributes look at a common configuration value to determine if it should capture them, OR
  2. have each instrumentation always capture them, but then have something later (the exporter?) filter them out based on configuration
    ?

Also looking for suggestions on what the configuration property should be named.
If we go with option 1 (where instrumentations check a config property), then perhaps otel.instrumentation.common.capture-enduser.enabled=true/false

@mateuszrzeszutek
Copy link
Member

IMO option 1 should be chosen as the solution here -- treat this attribute as an opt-in thing, and only collect it when the user explicitly configures the agent to do so.

If we go with option 1 (where instrumentations check a config property), then perhaps otel.instrumentation.common.capture-enduser.enabled=true/false

SGTM 👍

@philsttr
Copy link
Contributor Author

For example, i've seen production queue consumer code that manually calls SecurityContextHolder#setContext. Including these attributes as baggage on HTTP requests by that queue consumer could be unexpected.

I agree. I have seen a lot of code other than inbound authentication code call SecurityContextHolder#setContext. So I think if we were to instrument that method, it would catch too many use cases outside of the scope of setting the enduser.* attributes for SERVER spans.

So maybe more focused instrumentation is needed to avoid that, like on the specific spring security oauth2 resource server http filters.

I agree with this. After looking through spring security code a bit, I think the authentication related filters would be the best place to capture the enduser.* attributes for SERVER spans. Here are the the locations that I have found to capture the enduser.* attributes for the various types of authentication:

Reactive:

Servlet:

@trask
Copy link
Member

trask commented Oct 23, 2023

IMO option 1 should be chosen as the solution here -- treat this attribute as an opt-in thing, and only collect it when the user explicitly configures the agent to do so.

related: open-telemetry/semantic-conventions#128 (comment)

The guidance to not generate PII by default is temporary, pending better enable/disable features in the SDK for full user control.

@philsttr
Copy link
Contributor Author

I just filed spring-projects/spring-security#14046 to see if the Spring Security team would consider adding a hook that could be used by instrumentation to capture these attributes, or to see if they would recommend a different approach.

@philsttr
Copy link
Contributor Author

philsttr commented Oct 25, 2023

New approach, as suggested by the spring-security folks...

Add a filter into the spring security filter chain. The new filter is ordered after the filters that setup the spring security context. Therefore, the new filter can retrieve the Authentication and populate the enduser.* attributes.

More specifically

  • Servlet apps: instrument org.springframework.security.config.annotation.web.builders.HttpSecurity.build() to add a filter before the AuthorizationFilter
  • Reactive apps: instrument org.springframework.security.config.web.server.ServerHttpSecurity.build() to add a filter before the logout filter

@philsttr
Copy link
Contributor Author

philsttr commented Nov 2, 2023

For reference: #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
contribution welcome Request makes sense, maintainers probably won't have time, contribution would be welcome enhancement New feature or request new instrumentation
Projects
None yet
Development

No branches or pull requests

4 participants