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 enduser attributes in Spring Security #9777

Merged

Conversation

philsttr
Copy link
Contributor

@philsttr philsttr commented Oct 30, 2023

Adds library and javaagent instrumentation for spring-security-config to capture enduser.* semantic attributes.

The library instrumentation provides:

  • a Servlet Filter and a WebFlux WebFilter to capture enduser.* semantic attributes from Spring Security Authentication objects.
  • Customizer implementations to insert those filters into the security filter chains created by HttpSecurity and ServerHttpSecurity, respectively.

The javaagent instrumentation applies the Customizer implementations in the build() methods of HttpSecurity and ServerHttpSecurity.

The automatic instrumentation is disabled by default, due to the following requirement in the specification:

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.

Since this requirement is common to any automatic instrumentation that captures enduser.* attributes, the following new common configuration properties are introduced:

Property Type Default Description
otel.instrumentation.common.enduser.id.enabled Boolean false Whether to capture enduser.id semantic attribute.
otel.instrumentation.common.enduser.role.enabled Boolean false Whether to capture enduser.role semantic attribute.
otel.instrumentation.common.enduser.scope.enabled Boolean false Whether to capture enduser.scope semantic attribute.

In addition, the following new spring-security specific configuration properties are introduced:

Property Type Default Description
otel.instrumentation.spring-security.enduser.role.granted-authority-prefix String ROLE_ Prefix of granted authorities identifying roles to capture in the enduser.role semantic attribute.
otel.instrumentation.spring-security.enduser.scope.granted-authority-prefix String SCOPE_ Prefix of granted authorities identifying scopes to capture in the enduser.scopes semantic attribute.

@philsttr philsttr changed the title [DRAFT - Help Needed] Capture enduser attributes in Spring Security [DRAFT] Capture enduser attributes in Spring Security Oct 30, 2023
@philsttr philsttr force-pushed the spring_security_config_instrumentation branch from d0245b5 to d74065f Compare October 30, 2023 18:28
@philsttr
Copy link
Contributor Author

Looking for input on how to fix the failing muzzle verification.
It's missing some classes from jakarta.servlet:jakarta.servlet-api and org.springframework.security:spring-security-web, but I'm not sure how/where to make those available.

@philsttr
Copy link
Contributor Author

philsttr commented Oct 31, 2023

I'm not sure how/where to make those available.

Figured it out. Needed to add a few extraDependency entries in the muzzle section.

@philsttr philsttr changed the title [DRAFT] Capture enduser attributes in Spring Security Capture enduser attributes in Spring Security Oct 31, 2023
@philsttr philsttr marked this pull request as ready for review October 31, 2023 13:09
@philsttr philsttr requested a review from a team October 31, 2023 13:09
@philsttr
Copy link
Contributor Author

philsttr commented Oct 31, 2023

Looking for opinions on the flags. I want the ability to control whether each enduser attribute is captured. There are a couple approaches of doing that.

Approaches:

Approach A (which is currently implemented in this PR)

  • One "main" flag to turn on enduser.* attributes (otel.instrumentation.common.enduser.enabled). Default false.
  • Separate individual flags to turn off specific attributes (e.g. otel.instrumentation.common.enduser.scope.enabled). Default true. Only considered if the main flag is enabled

Approach B (alternative)

  • No main flag
  • Separate individual flags to turn on specific attributes (e.g. otel.instrumentation.common.enduser.scope.enabled). Default false.

Use Cases:

Use case 1: Enable all enduser.* attributes

With Approach A:

  • otel.instrumentation.common.enduser.enabled=true

With Approach B:

  • otel.instrumentation.common.enduser.id.enabled=true
  • otel.instrumentation.common.enduser.role.enabled=true
  • otel.instrumentation.common.enduser.scope.enabled=true

Use case 2: Enable only the enduser.id attribute

With Approach A:

  • otel.instrumentation.common.enduser.enabled=true
  • otel.instrumentation.common.enduser.role.enabled=false
  • otel.instrumentation.common.enduser.scope.enabled=false

With Approach B:

  • otel.instrumentation.common.enduser.id.enabled=true

What is your opinion? A or B?

@philsttr philsttr force-pushed the spring_security_config_instrumentation branch from 03b568e to 8aa5b21 Compare October 31, 2023 15:29
Adds library and javaagent instrumentation for spring-security-config
to capture `enduser.*` semantic attributes.

The library instrumentation provides:
* a Servlet `Filter` and a WebFlux `WebFilter` to capture `enduser.*`
  semantic attributes from Spring Security `Authentication` objects.
* `Customizer` implementations to insert those filters into the
  security filter chains created by `HttpSecurity` and
  `ServerHttpSecurity`, respectively.

The javaagent instrumentation applies the `Customizer` implementations
in the `build()` methods of `HttpSecurity` and `ServerHttpSecurity`.

The automatic instrumentation is disabled by default, due to the
following requirement in the specification:

> 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.

Since this requirement is common to any automatic instrumentation that
captures `enduser.*` attributes, the following new common configuration
properties are introduced:

* `otel.instrumentation.common.enduser.enabled` - default false.
  Whether to capture `enduser.*` semantic attributes.
  Must be set to true to capture any `enduser.*` attributes.
* `otel.instrumentation.common.enduser.id.enabled` - default true.
  Whether to capture `enduser.id` semantic attribute.
  Only takes effect if `otel.instrumentation.common.enduser.enabled`
  is true.
* `otel.instrumentation.common.enduser.role.enabled` - default true.
  Whether to capture `enduser.role` semantic attribute.
  Only takes effect if `otel.instrumentation.common.enduser.enabled`
  is true.
* `otel.instrumentation.common.enduser.scope.enabled` - default true.
  Whether to capture `enduser.scope` semantic attribute.
  Only takes effect if `otel.instrumentation.common.enduser.enabled`
  is true.

In addition, the following new spring-security specific configuration
properties are introduced:

* `otel.instrumentation.spring-security.enduser.role.granted-authority-prefix`
  default `ROLE_`.
  Prefix of granted authorities identifying roles to capture in the
  `enduser.role` semantic attribute.
* `otel.instrumentation.spring-security.enduser.scope.granted-authority-prefix`
  default `SCOPE_`
  Prefix of granted authorities identifying scopes to capture in the
  `enduser.scopes` semantic attribute.
@philsttr philsttr force-pushed the spring_security_config_instrumentation branch from 8aa5b21 to 82c6dd9 Compare October 31, 2023 15:39
@trask
Copy link
Member

trask commented Oct 31, 2023

I want the ability to control whether each enduser attribute is captured

can you describe a bit about why you want this level of control? do you think this is a general need? thx

@philsttr
Copy link
Contributor Author

philsttr commented Oct 31, 2023

In my particular case, I do not want enduser.scope captured because a lot of the tokens used in our systems have many scopes (50+), and we don't want them captured on every span, because we pay based on volume.

We also don't particularly care about enduser.role, since our systems don't make any decisions based on it.

I expect others to have similar use cases.

@trask
Copy link
Member

trask commented Oct 31, 2023

thanks, that makes sense, Approach B seems good to me as it optimizes this use case, and is less complex compared to opt-in + opt-out

@philsttr
Copy link
Contributor Author

philsttr commented Oct 31, 2023

thanks, that makes sense, Approach B seems good to me as it optimizes this use case, and is less complex compared to opt-in + opt-out

Ok. I agree. I started leaning that direction after trying to document Approach A. Hah.

I'll convert this PR to Approach B

@philsttr
Copy link
Contributor Author

I've switched the PR over to Approach B and rebased on main, and all checks are passing.

This PR is now ready for review.

philsttr added a commit to philsttr/opentelemetry-java-instrumentation that referenced this pull request Oct 31, 2023
This brings the flag name into alignment with open-telemetry#9777
@philsttr philsttr requested a review from laurit November 6, 2023 11:40
@philsttr
Copy link
Contributor Author

Any chance this can get merged for a November release?

Copy link
Member

@trask trask left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a couple of questions about otel.instrumentation.spring-security.enduser.role.granted-authority-prefix (and similar for scopes)

  • why only report the suffixes for matched roles? I think reporting the full role for matches will be clearer on the resulting telemetry (since consumers of the telemetry won't know the prefix used)
  • why not otel.instrumentation.common.enduser.role.granted-authority-prefix? (or maybe just otel.instrumentation.common.enduser.role.prefix)

@philsttr
Copy link
Contributor Author

Excellent questions.

The short answer is that the prefix/suffix logic is to compensate for how roles and scopes are modeled as granted authorities in the spring security implementation.

Now for the long answer...

To explain, consider an OAuth scope named user:read. In an OAuth token, this scope is identified as user:read. However, when spring security creates a granted authority for that scope, the granted authority will be SCOPE_user:read. The SCOPE_ prefix is not actually part of the scope name. It's an implementation detail within spring security's granted authority usage.

Roles are similar. A granted authority named ROLE_ADMIN is for a role named ADMIN

And since the enduser.role and enduser.scope are representations of roles and scopes, and not representations of spring-security's granted authority concept, the instrumentation should strip the prefixes of the granted authority to get the underlying name of the role / scope.

Regarding the configuration property names... The prefix/suffix concept is specific to spring security's granted authority implementation. Since this is spring-security specific, the configuration values should be prefixed with otel.instrumentation.spring-security rather than otel.instrumentation.common. I also think granted-authority needs to be in the name of the properties, because the prefix/suffix is specific to translating from granted authorities. They are not role prefixes or scope prefixes, but rather the prefix used to identify granted authorities that represent roles/scopes.

@trask trask added this to the v1.32.0 milestone Nov 14, 2023
@trask
Copy link
Member

trask commented Nov 14, 2023

thanks! one more question

The SCOPE_ prefix is not actually part of the scope name. It's an implementation detail within spring security's granted authority usage.

do we need to expose otel.instrumentation.common.enduser.role.granted-authority-prefix as a configurable property? when would a user set it (as opposed to using the default value)?

@philsttr
Copy link
Contributor Author

philsttr commented Nov 14, 2023

Spring Security allows the user to use different granted authority prefixes. Unfortunately, I could not find a way for the instrumentation to definitively determine if the user has configured spring security to use a different prefix. Therefore, I had to fallback on allowing the user to configure the prefix that OTel looks for via an OTel config property.

@trask trask merged commit 6ed3239 into open-telemetry:main Nov 14, 2023
46 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants