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

Support absolute URI’s in authentication success redirects for WebFlux #11656

Closed
DidierLoiseau opened this issue Aug 1, 2022 · 8 comments
Closed
Assignees
Labels
in: web An issue in web modules (web, webmvc) status: declined A suggestion or change that we don't feel we should currently apply type: enhancement A general enhancement

Comments

@DidierLoiseau
Copy link

DidierLoiseau commented Aug 1, 2022

Expected Behavior

RedirectServerAuthenticationSuccessHandler/ServerRequestCache.getRedirectUri() should allow to redirect using an absolute URL (e.g. http://localhost/secured-path) so that reverse proxies can automatically rewrite the location header, like with Web MVC (SavedRequestAwareAuthenticationSuccessHandler / RequestCache).

Current Behavior

The WebFlux success handler uses a relative URI, such as /secured-path. Reverse proxies will not rewrite those paths by default as they can’t know to what this path is relative (as I understand it, tested with Nginx’s proxy_pass setup).

I understand from #7273 that this behavior is intentional, however it would be good to make it easier to change the behavior. Currently the only solution seems to be to provide a custom ServerRequestCache or a custom RedirectServerAuthenticationSuccessHandler, as a user did in this SO answer. Moreover neither WebSessionServerRequestCache nor CookieServerRequestCache can be extended to customize this behavior during the saveRequest() call because they both use static methods to build the stored URL, and the attribute/cookie name is private.

Context

We are currently upgrading from Zuul 1 to Spring Cloud Gateway. As we deploy it behind Nginx in our test environment, we noticed that it does not rewrite relative location headers by default, so the raw internal value is forwarded. As a workaround, it is possible to force Nginx to rewrite it using proxy_redirect (which also converts the location to an absolute URI):

location /api/ {
    proxy_pass http://gateway:1234/;
    proxy_redirect default;
    proxy_redirect / /api/;
}

but we would rather avoid the trouble of asking our customer to change their reverse proxy configuration (this would involve another team to whom we need to explain the issue, it’s unlikely to work on first try etc. – we are likely to end up implementing a workaround in our gateway anyway).

@DidierLoiseau DidierLoiseau added status: waiting-for-triage An issue we've not yet triaged type: enhancement A general enhancement labels Aug 1, 2022
@sjohnr
Copy link
Member

sjohnr commented Aug 22, 2022

Hi @DidierLoiseau, thanks for reaching out! Sorry for the delay in responding.

I'd like to understand the issue better.

RedirectServerAuthenticationSuccessHandler/ServerRequestCache.getRedirectUri() should allow to redirect using an absolute URL

This is possible as the stackoverflow post mentioned. For example:

public class AbsoluteUrlRedirectStrategy implements ServerRedirectStrategy {

	private final ServerRedirectStrategy delegate = new DefaultServerRedirectStrategy();

	@Override
	public Mono<Void> sendRedirect(ServerWebExchange exchange, URI location) {
		return Mono.fromCallable(() -> this.createLocation(exchange, location))
			.flatMap(redirectUri -> this.delegate.sendRedirect(exchange, redirectUri));
	}

	private URI createLocation(ServerWebExchange exchange, URI location) {
		String url = location.toASCIIString();
		String contextPath = null;
		if (url.startsWith("/")) {
			contextPath = exchange.getRequest().getPath().contextPath().value();
		}
		return UriComponentsBuilder.fromHttpRequest(exchange.getRequest())
			.replacePath(contextPath)
			.path(url)
			.build()
			.toUri();
	}

}

Since the ServerRedirectStrategy allows this through customization, are you looking for an implementation that provides this out of the box?

it would be good to make it easier to change the behavior.

Is it just the RedirectServerAuthenticationSuccessHandler that you are looking to customize? Or are their other components that send redirects that you need to customize?

As an aside: I look at adding absolute URLs to the location header as a cross-cutting concern that the gateway could handle without any impact on Spring Security configuration. Since Spring Security is consistent in how it reflects the Location header, it does seem to make sense to just handle this at the gateway level (in Spring Cloud Gateway).

@sjohnr sjohnr self-assigned this Aug 22, 2022
@sjohnr sjohnr added status: waiting-for-feedback We need additional information before we can continue in: web An issue in web modules (web, webmvc) and removed status: waiting-for-triage An issue we've not yet triaged labels Aug 22, 2022
@DidierLoiseau
Copy link
Author

Thanks @sjohnr for your answer. This ticket is indeed just about making it easier do this, ideally through configuration instead of code, in order to get the same behavior as Spring Security provides for Spring MVC. I’m not sure this should be specific to the gateway since this affects the general behavior of the ServerAuthenticationSuccessHandler.

As a side note, in the end we opted for extending the WebSessionServerRequestCache instead:

import static org.springframework.security.web.util.UrlUtils.buildFullRequestUrl;

import java.net.URI;

import reactor.core.publisher.Mono;

import org.springframework.security.web.server.savedrequest.WebSessionServerRequestCache;
import org.springframework.web.server.ServerWebExchange;

class AbsoluteRedirectWebSessionServerRequestCache extends WebSessionServerRequestCache {
	@Override
	public Mono<URI> getRedirectUri(ServerWebExchange exchange) {
		return super.getRedirectUri(exchange).map(redirectURI -> {
			URI uri = exchange.getRequest().getURI();
			return URI.create(
				  buildFullRequestUrl(uri.getScheme(), uri.getHost(), uri.getPort(), redirectURI.getPath(),
						redirectURI.getQuery()));
		});
	}
}

Ideally, I suppose this should be done by changing the behavior of saveRequest() instead, to make sure the value is kept from the initial request, like HttpSessionRequestCache does for MVC. But as I said in the description it cannot be done cleanly: sessionAttrName and pathInApplication() are both private, so you’d have to reimplement the whole saveRequest() method, and if you don’t want to rely on private internal values of WebSessionServerRequestCache to get the value of sessionAttrName, you’d probably have to reimplement the whole class (especially since you cannot actually change the value of sessionAttrName, as far as I can tell).

If I’m not wrong, the behavior I’d like to have could be implemented by changing the line

String requestPath = pathInApplication(exchange.getRequest());

to something like

String requestPath = absolute ? exchange.getRequest().getURI().toString() : pathInApplication(exchange.getRequest());

and provide a way to configure this absolute toggle.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Aug 23, 2022
@sjohnr
Copy link
Member

sjohnr commented Aug 23, 2022

Thanks @DidierLoiseau.

It sounds like you are solely focused on the redirect after authentication success, correct?

This ticket is indeed just about making it easier do this, ideally through configuration instead of code, in order to get the same behavior as Spring Security provides for Spring MVC.

This can be configured in a single spot in this case, which would be customizing the ServerAuthenticationSuccessHandler. For example, with .formLogin():

@Configuration
@EnableWebFluxSecurity
public class SecurityConfiguration {

	@Bean
	public SecurityWebFilterChain securityWebFilterChain(ServerHttpSecurity http) {
		http
			.authorizeExchange((authorizeExchange) -> authorizeExchange
				.anyExchange().authenticated()
			)
			.formLogin((formLogin) -> formLogin
				.authenticationSuccessHandler(authenticationSuccessHandler())
			);
		return http.build();
	}

	private ServerAuthenticationSuccessHandler authenticationSuccessHandler() {
		RedirectServerAuthenticationSuccessHandler authenticationSuccessHandler =
			new RedirectServerAuthenticationSuccessHandler();
		// see above definition of AbsoluteUrlRedirectStrategy
		authenticationSuccessHandler.setRedirectStrategy(new AbsoluteUrlRedirectStrategy());

		return authenticationSuccessHandler;
	}

}

Perhaps I'm missing why you chose to customize the WebSessionServerRequestCache instead? It's also worth noting that Spring MVC support will change to relative URLs as well per gh-7273. Given that, I'm not sure if the framework supporting absolute URLs would be a goal worth pursuing any longer. I'll leave this open and we can discuss this point further.

@sjohnr sjohnr added for: team-attention This ticket should be discussed as a team before proceeding and removed status: feedback-provided Feedback has been provided labels Aug 23, 2022
@DidierLoiseau
Copy link
Author

DidierLoiseau commented Aug 26, 2022

It sounds like you are solely focused on the redirect after authentication success, correct?

Indeed.

Perhaps I'm missing why you chose to customize the WebSessionServerRequestCache instead?

Well, both approaches are possible, but the ServerRequestCache is the equivalent of the MVC RequestCache¹, which is the one providing absolute URL’s, so it seemed more consistent to do it that way. As I mentioned in my previous comment, I suppose a proper implementation should save the absolute URL from the original request and not try to rebuild it at redirect time, and this can only be implemented in the request cache. I don’t know if there are scenarios in which this could be problematic (maybe in some clustered environments with synchronized sessions?).

Also note that configuring the request cache is just as easy with ServerHttpSecurity.requestCache().

I suppose if #7273 is implemented, having an easy way to switch back to absolute URL’s would be welcome too, considering the burden it creates with reverse proxies (or at least, with Nginx).

¹ Note that in both cases those caches appear to be there solely for authentication purposes, according to their Javadoc.

@sjohnr
Copy link
Member

sjohnr commented Aug 26, 2022

note that configuring the request cache is just as easy with ServerHttpSecurity.requestCache().

True! However, I was focused on the case where more than just saved requests would be used as redirects.

Even if your particular use case doesn't require the RedirectStrategy to be customized, I would think a general solution for others would require this. For that reason, I don' think a general solution would focus on the request cache.

I suppose if #7273 is implemented, having an easy way to switch back to absolute URL’s would be welcome too, considering the burden it creates with reverse proxies (or at least, with Nginx).

Modern browsers can handle relative URLs in the Location header, correct? What was the behavior or result in your test environment when a relative Location header was returned?

@DidierLoiseau
Copy link
Author

Even if your particular use case doesn't require the RedirectStrategy to be customized, I would think a general solution for others would require this. For that reason, I don' think a general solution would focus on the request cache.

I don’t know why the MVC RequestCache is handling it in the first place, but I guess there must be a reason to store the full original request and not just its path & query params. This means that there might be use cases where handling it in the RedirectStrategy would not work.

What was the behavior or result in your test environment when a relative Location header was returned?

See initial issue description but to summarize: Nginx’s proxy_pass does not rewrite them (by default), so Spring’s raw relative URL is returned to the browser, and that is not a valid URL since it is only relative to the Spring application server’s internal hostname, not the externally-facing domain.

@sjohnr
Copy link
Member

sjohnr commented Aug 29, 2022

Spring’s raw relative URL is returned to the browser, and that is not a valid URL since it is only relative to the Spring application server’s internal hostname, not the externally-facing domain.

Thanks for clarifying. If your nginx proxy is rewriting all URLs before it gets to Spring Cloud Gateway, I could see how this is a problem.

@sjohnr
Copy link
Member

sjohnr commented Sep 1, 2022

Hi @DidierLoiseau,

I spoke to the team about this issue and I wanted to give you an update. Regarding Spring WebFlux specifically, we believe that this issue is broader than authentication success, and in fact is even broader than Spring Security. If your nginx proxy has issues with relative redirects, the best place to add a configuration option would be in Spring Framework itself, as this is managed in org.springframework.http.server.reactive.ServerHttpResponse. I would suggest opening an issue with spring-framework to discuss possible enhancements.

In the meantime, a passive solution would be to add a WebFilter to handle this. For example:

@Component
@Order(-100)
public class AbsoluteUrlLocationHeaderWebFilter implements WebFilter {

	@Override
	public Mono<Void> filter(ServerWebExchange exchange, WebFilterChain filterChain) {
		return filterChain.filter(exchange).then(checkLocation(exchange));
	}

	private Mono<Void> checkLocation(ServerWebExchange exchange) {
		return Mono.fromRunnable(() -> {
			HttpHeaders headers = exchange.getResponse().getHeaders();
			URI location = headers.getLocation();
			if (location != null) {
				headers.setLocation(createLocation(exchange, location));
			}
		});
	}

	private URI createLocation(ServerWebExchange exchange, URI location) {
		String url = location.toASCIIString();
		if (url.startsWith("/")) {
			return UriComponentsBuilder.fromHttpRequest(exchange.getRequest())
				.query(null)
				.fragment(null)
				.replacePath(url)
				.build()
				.toUri();
		}
		return location;
	}

}

I'm going to close this issue for now as we probably wouldn't pursue a specific change for success redirects and application-wide redirect handling for Spring WebFlux could be addressed at the framework level.

@sjohnr sjohnr closed this as completed Sep 1, 2022
@sjohnr sjohnr added status: declined A suggestion or change that we don't feel we should currently apply and removed for: team-attention This ticket should be discussed as a team before proceeding labels Sep 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: web An issue in web modules (web, webmvc) status: declined A suggestion or change that we don't feel we should currently apply type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

3 participants