support x-forwarded-host

[fkowal]

when running spring boot app behing a proxy
request goes to
https://example.com

then it's forwarded to spring boot app, request looks like this

Host: springapp.host
X-Forwarded-Host: example.com
X-Forwarded-Proto: https

When using embedded Tomat
org/springframework/boot/autoconfigure/web/ServerProperties.class

configures RemoteIpValve, that handles X-Forwarded-Proto
but X-Forwarded-Host is not handled

later when spring security handles authentication
org/springframework/security/web/authentication/LoginUrlAuthenticationEntryPoint.java
redirect to login form is not built incorrectly
Using the
Host: header
and X-Forwarded-Host is ignored

[cemo]

I don't know how Tomcat and Undertow handling but here is what I have done for Jetty. Note that Jetty has internal support for this.

@Configuration
public class JettyForwardedAutoConfiguration implements EmbeddedServletContainerCustomizer {

   @Override
   public void customize(ConfigurableEmbeddedServletContainer container) {
      JettyEmbeddedServletContainerFactory containerFactory = (JettyEmbeddedServletContainerFactory) container;
      containerFactory.addServerCustomizers(server -> {
         for (Connector connector : server.getConnectors()) {
            ConnectionFactory connectionFactory = connector.getDefaultConnectionFactory();
            if(connectionFactory instanceof HttpConnectionFactory) {
               HttpConnectionFactory defaultConnectionFactory = (HttpConnectionFactory) connectionFactory;
               HttpConfiguration httpConfiguration = defaultConnectionFactory.getHttpConfiguration();
               httpConfiguration.addCustomizer(new ForwardedRequestCustomizer());
            }
         }
      });
   }
}

I would be happy to be supported default by Jetty and others.

Spring's org.springframework.web.servlet.support.ServletUriComponentsBuilder has also support by default. I believe that it make sense supporting by default.

+1

[fkowal]

this is the same issue in different context https://jira.spring.io/browse/SPR-10110

[wilkinsona]

I think that this really ought to be fixed in Spring Security. A fix there will benefit everyone who uses Spring Security and not just those who also use Spring Boot. @rwinch?

[cemo]

@wilkinsona I had already filed an issue similar to this one last year. Please see: https://jira.spring.io/browse/SEC-2526

[wilkinsona]

Interesting. Unless I've missed something, Tomcat's RemoteIpValve doesn't support doing anything with the X-Forwarded-Host header so I don't think it'll help. Perhaps also worth noting is that Spring HATEOAS does look at X-Forwarded-Host and, as already noted above, Spring Framework does too. I think it would be good for Spring Security to be consistent with these other projects, but lets see what @rwinch says.

Edit - @cemo I've just noticed that your issue was about X-Forwarded-Proto which RemoteIpValve does support, not about X-Forwarded-Host. I'm not surprised that Rob pointed you to RemoteIpValve for X-Forwarded-Proto.

[fkowal]

i filed a request for tomcat to support this header https://bz.apache.org/bugzilla/show_bug.cgi?id=57665
don't know what good that will do

[rwinch]

To me it seems undesirable that the logic to resolve the correct host is duplicated and throughout the various layers (HATEOAS, Framework, Security, etc) of Spring. This logic makes it difficult to maintain.

Additionally, it makes it difficult to keep up with various proxy. For example HAProxy, nginx, ELB, etc support Proxy Protocol.

In all honesty, I've never used Proxy Protocol so I am not certain how it impacts things with a Servlet Container. However, the fact that there are many layers of Spring performing the same logic and that there are multiple ways that logic might be performed hints to me that something is amiss.

[wilkinsona]

@rwinch There's no need to duplicate the logic. It's only in Spring Framework and Spring HATEOAS as the latter got there first. You can reuse the logic from Spring Framework by calling ServletUriComponentsBuilder.fromRequest.

Making a change in Spring Security that benefits many people is surely preferable to many people duplicating the configuration of a Valve, header rewriting, etc

[rwinch]

@wilkinsona Thanks for pointing this out (somehow I had missed it). We could likely use this with Spring Security 4.0+, but Spring Security 3.2.x still supports Spring 3.x line which does not appear to have this functionality. I created https://jira.spring.io/browse/SEC-2898

[wilkinsona]

Closing in favour of SEC-2898.

[sfussenegger]

@wilkinsona I'd like to reopen this ticket as this can't work without support by Tomcat itself. This can be easily demonstrated by a simple redirect:

@RequestMapping("/redirect")
public String redirect() {
    return "redirect:/";
}

$ curl -si \
  -H "X-Forwarded-Proto: https" \
  -H "X-Forwarded-Host: example.com" \
  -H "X-Forwarded-Port: 443" \
  127.0.0.1:8080/redirect
HTTP/1.1 302 Found
Location: https://127.0.0.1/

expected location would be https://example.com/

The situation is however even worse for a context redirect caused by this code in TomcatEmbeddedServletContainerFactory (introduced in 43ed824):

    context.setUseRelativeRedirects(false);
    context.setMapperContextRootRedirectEnabled(true);

the request won't even reach the RemoteIpValve configured by server.use-forward-headers:

$ curl -si \
  -H "X-Forwarded-Proto: https" \
  -H "X-Forwarded-Host: example.com" \
  -H "X-Forwarded-Port: 443" \
  127.0.0.1:8080/context
HTTP/1.1 302 Found
Location: http://127.0.0.1:8080/context/

expected location would be https://example.com/context/

[wilkinsona]

I'd like to reopen this ticket as this can't work without support by Tomcat itself.

I'm not sure I understand. @sfussenegger if this can't work without support by Tomcat itself, how will reopening this issue help?

The situation is however even worse for a context redirect caused by this code in TomcatEmbeddedServletContainerFactory (introduced in 43ed824)

IIRC, the code in 43ed824 simply reinstated Tomcat's default behaviour that had been changed inadvertently and was then reverted in a subsequent release. I've just looked at Tomcat 8.0.33 and mapperContextRootRedirectEnabled is true by default so we could revert the change and it would have no effect.

[sfussenegger]

It could of course be a new ticket instead. But the main point is that support for X-Forwarded-* headers is inconsistent.

The context redirect is a topic of its own as it bypasses all of them as it bypasses Tomcat's RemoteIpValve completely, making server.use-forward-headers having no effect. I think what could be done there is adding a root context with the same valve. As far as I've seen that's not possible right now without patching the code but I might be wrong.

The bad redirect caused by redirect:/ is probably a bug too as you've argued that spring code should use UriComponentsBuilder to generate absolute URLs. Currently it seems to rely on the servlet container generating it.

Back to X-Forwarded-Host though which is the subject of this ticket. I do believe that support should be implemented equal to alll the other X-Forwarded-* headers. Tomcat's RemoteIpValve makes sure that X-Forwarded-Port is returned by ServletRequest.getServerPort(), X-Forwarded-Proto effects ServletRequest.isSecure(), but X-Forwarded-Host is not returned by ServletRequest.getServerName() which I think is non-obvious. This is somewhat proven by @dsyer suggesting to enable server.use-forward-headers to fix spring-cloud/spring-cloud-netflix#1108.

As I've said, Tomcat does not support it but with a custom valve it can be added rather easily:

    // copied from spring-cloud/spring-cloud-netflix#1108
    @Bean
    public EmbeddedServletContainerCustomizer customizer() {
        return (final ConfigurableEmbeddedServletContainer container) -> {
            ((TomcatEmbeddedServletContainerFactory) container).addContextValves(new ValveBase() {

                @Override
                public void invoke(final Request request, final Response response) throws IOException, ServletException {

                    final MessageBytes serverNameMB = request.getCoyoteRequest().serverName();
                    String originalServerName = null;
                    final String forwardedHost = request.getHeader("X-Forwarded-Host");
                    if (forwardedHost != null) {
                        originalServerName = serverNameMB.getString();
                        serverNameMB.setString(forwardedHost);
                    }

                    try {
                        getNext().invoke(request, response);
                    } finally {
                        if (forwardedHost != null) {
                            serverNameMB.setString(originalServerName);
                        }
                    }
                }
            });
        };
    }

Personally I feel that this should be added if server.use-forward-headers is enabled or that there is at least another option to do so. You could probably argue that I could do this myself as it's already possible. I do however feel that others will run into the same issue I had. As I've spent quite some time figuring this out I suppose others will too. I think the existence of this ticket already suggests that.

[timbuethe]

@sfussenegger's solution should look something like this if you're on Spring Boot 2

@Bean
    public WebServerFactoryCustomizer<TomcatServletWebServerFactory> customizer() {
        return (final TomcatServletWebServerFactory factory) -> {
            factory.addContextValves(new ValveBase() {
               ...
            });
        };
    }