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

Unset forward header strategy in kubernetes environment eats X-Forwarded-* headers in Spring Boot 2.2 #19333

Closed
bric3 opened this issue Dec 9, 2019 · 2 comments
Assignees
Labels
type: regression A regression from a previous release
Milestone

Comments

@bric3
Copy link

bric3 commented Dec 9, 2019

I'm working on applications that requires access to X-Forwarded-* headers, unfortunately Spring Boot 2.2 changed the way it auto-configures itself, and those are not anymore available in the custom code unless you dive in to understand the change.

In spring boot 2.1 when the server.use-forward-headers property was not set, Spring Boot did not configure the org.apache.catalina.valves.RemoteIpValve from Tomcat when the application was running within a cloud provider environment.

This changed in Spring Boot 2.2, when the new property server.forward-headers-strategy is not configured, i.e. it's value is org.springframework.boot.autoconfigure.web.ServerProperties.ForwardHeadersStrategy.NONE, then the new code always tries to detect a cloud provider via org.springframework.boot.cloud.CloudPlatform.getActive(Environment) (using the environment variables). In particular when Kubernetes is detected it configures the org.apache.catalina.valves.RemoteIpValve (which the same as if server.forward-headers-strategy property was set to org.springframework.boot.autoconfigure.web.ServerProperties.ForwardHeadersStrategy.NATIVE).

This Tomcat Valve has a side effect : this class replaces the value in the corresponding attribute of javax.servlet.ServletRequest and removes the X-Forwarded-* header. On the other hand switching server.forward-headers-strategy to org.springframework.boot.autoconfigure.web.ServerProperties.ForwardHeadersStrategy.FRAMEWORK to prevent the auto-detection of the cloud platform does not help as well. This time it's the ForwardedHeaderFilter that has the same kind of side effect : this filter map all X-Forwarded-* headers to javax.servlet.ServletRequest attributes but X-Forwarded-For.

This is unfortunate as this isn't explicitly documented, and there's no way to disable this behavior by configuration. While I understand the value of the change as a whole, I believe this particular scenario could be seen as a regression that was introduced.

Currently the only hack to prevent this is to register a dummy of ForwardHeaderFilter

    @Bean
    public ForwardedHeaderFilter disabledForwardedHeaderFilter() {
        return new ForwardedHeaderFilter() {
            @Override
            protected boolean shouldNotFilter(HttpServletRequest request) {
                return true;
            }
        };
    }

What I think is missing in the current code 2.2.1.RELEASE are these things

  • org.springframework.boot.autoconfigure.web.ServerProperties.ForwardHeadersStrategy.FORCE_NONE
  • allow to configure ForwardHeaderFilter to not remove the headers

On another related topic, discovering this behavior and testing it (with unit test) was not straightforward. For example the code of CloudPlatform explicitly asks for StandardEnvironment.SYSTEM_ENVIRONMENT_PROPERTY_SOURCE_NAME which obstructs the usual usage of the annotation @SpringBootTest(properties = {})

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Dec 9, 2019
@wilkinsona wilkinsona added type: regression A regression from a previous release and removed status: waiting-for-triage An issue we've not yet triaged labels Dec 9, 2019
@wilkinsona wilkinsona added this to the 2.2.x milestone Dec 9, 2019
@mbhave mbhave self-assigned this Dec 10, 2019
@kampka
Copy link

kampka commented Dec 12, 2019

FORCE_NONE is not a great name. NONE should simply do what the documentation claims it does: Ignore X-Forwarded-* headers.
What's required here is a new value like CLOUD_NATIVE which is much more self-explanatory.

@bric3
Copy link
Author

bric3 commented Dec 12, 2019

I totally agree !
I wasn't sure what was the expected behavior of NONE, should it detect automatically cloud provider ? Or simply because we don't want to break backward compatibilIty.
But if that's ok to keep NONE and I troduce something more explicit I'm all for it.

@mbhave mbhave added the for: team-attention An issue we'd like other members of the team to review label Jan 3, 2020
@mbhave mbhave modified the milestones: 2.2.x, 2.3.0.M1 Jan 3, 2020
@mbhave mbhave closed this as completed in c12a3f4 Jan 3, 2020
@mbhave mbhave modified the milestones: 2.3.0.M1, 2.2.3 Jan 3, 2020
@philwebb philwebb removed the for: team-attention An issue we'd like other members of the team to review label Jan 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: regression A regression from a previous release
Projects
None yet
Development

No branches or pull requests

6 participants