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

Improve "Sanitize Sensitive Values" section in reference documentation #39094

Closed
filpano opened this issue Jan 11, 2024 · 11 comments
Closed

Improve "Sanitize Sensitive Values" section in reference documentation #39094

filpano opened this issue Jan 11, 2024 · 11 comments
Assignees
Labels
type: documentation A documentation update
Milestone

Comments

@filpano
Copy link

filpano commented Jan 11, 2024

In Spring Boot 3, the Actuator security settings have changed somewhat, specifically with regard to the sanitization of potentially sensitive properties. Part of the reasoning for this (role-based vs. key-based) is given in #32156, which I do not question as being an improvement.

We currently use Spring Boot Admin and require an SSO administration login to see the UI which is populated by a service role with the monitoring role. This and other correctly configured monitoring tools now, per default, would show sensitive fields.

Specifically given a previous config of

management.endpoint.env.show-values=WHEN_AUTHORIZED

and a request matcher similar to the following:

httpSecurity. // ...
        .and()
        .authorizeHttpRequests()
        .requestMatchers("/actuator/**").hasAnyRole("MONITORING")
        .and() // ...

which would semantically map to the new:

management.endpoint.env.show-values=WHEN_AUTHORIZED
management.endpoint.env.roles=MONITORING

...would exhibit behaviour that is a drastic departure from the previous one w.r.t. sensitive information.

This is great for development - no doubt, but it was a bit of a shock to see it in action without an explicit and far more prominent warning that there is no default sanitizing function.

Since Spring Boot itself has a very opinionated approach to configuration, why do common-sense approaches allow us to use proper security practices (WHEN_AUTHORIZED with a specific system role) and still have potentially unsafe behaviour? The fact that a developer may need to be able to check a runtime configuration property does not immediately imply that they need access to sensitive values, with a sensible-looking configuration, per default.

IMO it would be much better to require a sanitizing function when WHEN_AUTHORIZED is used, and perhaps - at the very least, provide a NoOpSanitizingFunction that logs a suppressable warning. No one likes a silent breaking change.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jan 11, 2024
@filpano
Copy link
Author

filpano commented Jan 11, 2024

As a side point, in the migration guide, the options for the show-value property are given as follows:

NEVER - All values are sanitized (the default).

ALWAYS - All values are present in the output (sanitizing functions will apply).

WHEN_AUTHORIZED - Values are present in the output only if a user is authorized (sanitizing functions will apply).

(emphasis is mine)

In the current docs, the options are given as follows:

ALWAYS - all values are shown in their unsanitized form to all users

NEVER - all values are always sanitized (that is replaced by ******)

WHEN_AUTHORIZED - all values are shown in their unsanitized form to authorized users

(emphasis mine) including the following note:

When show-values is set to ALWAYS or WHEN_AUTHORIZED any sanitization applied by a SanitizingFunction will still be applied.

The migration guide is probably what most people read when upgrading to Spring Boot 3, and IMO it strongly implies that a sensible default, as previously, is being applied out of the box.

@filpano
Copy link
Author

filpano commented Jan 11, 2024

For anybody coming from a search engine and looking for an example on how to get the previous behaviour back, see the comment from @welsh here: #32156 (comment).

@wilkinsona
Copy link
Member

wilkinsona commented Jan 11, 2024

I've updated the migration guide to replace "sanitizing functions will apply" with "any user-defined sanitizing functions will still apply".

Specifically given a previous config of

management.endpoint.env.show-values=WHEN_AUTHORIZED

What do you mean by "previous config" here? management.endpoint.env.show-values is new in 3.0.

@filpano
Copy link
Author

filpano commented Jan 11, 2024

I've updated the migration guide to replace "sanitizing functions will apply" with "any user-defined sanitizing functions will still apply".

Great, thank you very much. I think this will help avoid a lot of misunderstandings.

What do you mean by "previous config" here? management.endpoint.env.show-values is new in 3.0.

Sorry, you are correct. It seems I was thinking of the previous Health-only configuration (i.e. management.endpoint.health.show-details), which used the same enum values as those of new Show enum.

I suppose this, in tandem with keys being sanitized previously and my wrong interpretation of the migration guide, is where most of the confusion stems from for me.

@philwebb philwebb added the for: team-meeting An issue we'd like to discuss as a team to make progress label Jan 11, 2024
@philwebb philwebb changed the title Include a default sanitizing function for actuator endpoints with sensitive information Improve "Sanitize Sensitive Values" section in reference documentation Jan 17, 2024
@philwebb philwebb added type: documentation A documentation update and removed status: waiting-for-triage An issue we've not yet triaged for: team-meeting An issue we'd like to discuss as a team to make progress labels Jan 17, 2024
@philwebb philwebb self-assigned this Jan 17, 2024
@philwebb philwebb added this to the 3.1.8 milestone Jan 17, 2024
@philwebb
Copy link
Member

@filpano I think our reference documentation could also be improved. I've repurposed this issue to do that.

@datagitlies
Copy link

datagitlies commented Jan 18, 2024

@philwebb beyond the documentation updates was any consideration given to provide a default SanitizingFunction or at least a reference implementation similar to the Sanitizer in spring-boot 2.7 that accepts a list of keysToSanitize? I know several engineers that are literally copying the Sanitizer logic into their codebase after migrating to spring-boot 3.X which would indicate there is a need for this.

EDIT: just some additional thoughts - I like the security changes (i.e. when_authorized) like is done with the health endpoints but env != health & configprops != health due to the sensitive nature of the properties within. Hence the need for a SanitizingFunction (one that is well-written / well-tested and provided by spring-boot out-of-the-box). Copying code is never ideal.

@wilkinsona
Copy link
Member

@datagitlies please see #33448.

@datagitlies
Copy link

@wilkinsona I still believe a better example SanitizingFunction should be "provided" (either in spring-boot-actuator or in documentation / a code example or sample ... basically somewhere!) Similar to how it was handled for the InMemoryHttpExchangeRepository where it's not enabled by default, but you can easily provide it (if desired). It avoids engineers having to copy write the code.

@wilkinsona
Copy link
Member

As explained in the issue to which I linked, we don't feel that we can do that:

We found it was impossible to write a SanitizingFunction that would consistently and correctly work against everything

We think it's safer if users provide their own, limited to their specific needs.

@datagitlies
Copy link

My expectation is not that spring-boot-actuator would or could provide a SanitizingFunction that will "correctly work against everything" ... I just want reasonable defaults. I don't think it's out of the question that a property called password could be sanitized by default -IF- I desire to enable a "default" SanitizingFunction provided by spring-boot-actuator (the one that doesn't exist but I'm proposing). The fact that anyone would have to spend time writing (or finding and copying) logic to sanitize that password property is what's at the core of this issue. Or am I missing something non-obvious here? Are there cases where the spring-boot 2.7 Sanitizer didn't work for something simple like a property called password?

@philwebb
Copy link
Member

@datagitlies I think I see what you're getting at. I've raised #39243 so we can consider our options.

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

No branches or pull requests

5 participants