Skip to content

Commit

Permalink
fix(core): remove ErrorPageSecurityFilter bean named errorPageSecurit…
Browse files Browse the repository at this point in the history
…yInterceptor (#1817)

* test(web): demonstrate bug in MultiAutoSupport

where handling of certain error responses generates html:

<!doctype html><html lang="en"><head><title>HTTP Status 400 – Bad Request</title><style type="text/css">body {font-family:Tahoma,Arial,sans-serif;} h1, h2, h3, b {color:white;background-color:#525D76;} h1 {font-size:22px;} h2 {font-size:16px;} h3 {font-size:14px;} p {font-size:12px;} a {color:black;} .line {height:1px;background-color:#525D76;border:none;}</style></head><body><h1>HTTP Status 400 – Bad Request</h1></body></html>

instead of json, and the following exception in the logs:

java.lang.UnsupportedOperationException: public abstract int javax.servlet.ServletRequest.getLocalPort() is not supported
    at org.springframework.security.web.FilterInvocation$UnsupportedOperationExceptionInvocationHandler.invoke(FilterInvocation.java:326)
    at jdk.proxy2/jdk.proxy2.$Proxy256.getLocalPort(Unknown Source)
    at javax.servlet.ServletRequestWrapper.getLocalPort(ServletRequestWrapper.java:329)
    at com.netflix.spinnaker.gate.config.MultiAuthSupport$1.lambda$requestMatcher$0(MultiAuthSupport.java:30)
    at org.springframework.security.web.DefaultSecurityFilterChain.matches(DefaultSecurityFilterChain.java:72)
    at org.springframework.security.web.access.RequestMatcherDelegatingWebInvocationPrivilegeEvaluator.getDelegate(RequestMatcherDelegatingWebInvocationPrivilegeEvaluator.java:120)
    at org.springframework.security.web.access.RequestMatcherDelegatingWebInvocationPrivilegeEvaluator.isAllowed(RequestMatcherDelegatingWebInvocationPrivilegeEvaluator.java:71)
    at org.springframework.boot.web.servlet.filter.ErrorPageSecurityFilter.isAllowed(ErrorPageSecurityFilter.java:88)
    at org.springframework.boot.web.servlet.filter.ErrorPageSecurityFilter.doFilter(ErrorPageSecurityFilter.java:76)
    at org.springframework.boot.web.servlet.filter.ErrorPageSecurityFilter.doFilter(ErrorPageSecurityFilter.java:70)
    at org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:178)
    at org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:153)
    at org.springframework.security.web.FilterChainProxy$VirtualFilterChain.doFilter(FilterChainProxy.java:337)
    at org.springframework.security.web.access.intercept.FilterSecurityInterceptor.invoke(FilterSecurityInterceptor.java:106)
    at org.springframework.security.web.access.intercept.FilterSecurityInterceptor.doFilter(FilterSecurityInterceptor.java:81)
    at org.springframework.security.web.FilterChainProxy$VirtualFilterChain.doFilter(FilterChainProxy.java:346)
    at org.springframework.security.web.access.ExceptionTranslationFilter.doFilter(ExceptionTranslationFilter.java:122)
    at org.springframework.security.web.access.ExceptionTranslationFilter.doFilter(ExceptionTranslationFilter.java:116)
    at org.springframework.security.web.FilterChainProxy$VirtualFilterChain.doFilter(FilterChainProxy.java:346)
    at org.springframework.security.web.session.SessionManagementFilter.doFilter(SessionManagementFilter.java:87)
    at org.springframework.security.web.session.SessionManagementFilter.doFilter(SessionManagementFilter.java:81)
    at org.springframework.security.web.FilterChainProxy$VirtualFilterChain.doFilter(FilterChainProxy.java:346)
    at org.springframework.security.web.authentication.AnonymousAuthenticationFilter.doFilter(AnonymousAuthenticationFilter.java:109)
    at org.springframework.security.web.FilterChainProxy$VirtualFilterChain.doFilter(FilterChainProxy.java:346)
    at org.springframework.web.filter.OncePerRequestFilter.doFilter(OncePerRequestFilter.java:102)
    at org.springframework.security.web.FilterChainProxy$VirtualFilterChain.doFilter(FilterChainProxy.java:346)
    at org.springframework.security.web.servletapi.SecurityContextHolderAwareRequestFilter.doFilter(SecurityContextHolderAwareRequestFilter.java:149)
    at org.springframework.security.web.FilterChainProxy$VirtualFilterChain.doFilter(FilterChainProxy.java:346)
    at org.springframework.security.web.savedrequest.RequestCacheAwareFilter.doFilter(RequestCacheAwareFilter.java:63)
    at org.springframework.security.web.FilterChainProxy$VirtualFilterChain.doFilter(FilterChainProxy.java:346)
    at org.springframework.web.filter.OncePerRequestFilter.doFilter(OncePerRequestFilter.java:102)
    at org.springframework.security.web.FilterChainProxy$VirtualFilterChain.doFilter(FilterChainProxy.java:346)
    at org.springframework.security.web.authentication.AbstractAuthenticationProcessingFilter.doFilter(AbstractAuthenticationProcessingFilter.java:219)
    at org.springframework.security.web.authentication.AbstractAuthenticationProcessingFilter.doFilter(AbstractAuthenticationProcessingFilter.java:213)
    at org.springframework.security.web.FilterChainProxy$VirtualFilterChain.doFilter(FilterChainProxy.java:346)
    at javax.servlet.FilterChain$doFilter.call(Unknown Source)
    at org.codehaus.groovy.runtime.callsite.CallSiteArray.defaultCall(CallSiteArray.java:47)
    at javax.servlet.FilterChain$doFilter.call(Unknown Source)
    at com.netflix.spinnaker.gate.security.oauth2.ExternalAuthTokenFilter.doFilter(ExternalAuthTokenFilter.groovy:65)
    at org.springframework.security.web.FilterChainProxy$VirtualFilterChain.doFilter(FilterChainProxy.java:346)
    at org.springframework.security.web.authentication.logout.LogoutFilter.doFilter(LogoutFilter.java:103)
    at org.springframework.security.web.authentication.logout.LogoutFilter.doFilter(LogoutFilter.java:89)
    at org.springframework.security.web.FilterChainProxy$VirtualFilterChain.doFilter(FilterChainProxy.java:346)
    at org.springframework.web.filter.OncePerRequestFilter.doFilter(OncePerRequestFilter.java:102)
    at org.springframework.security.web.FilterChainProxy$VirtualFilterChain.doFilter(FilterChainProxy.java:346)
    at org.springframework.security.web.context.SecurityContextPersistenceFilter.doFilter(SecurityContextPersistenceFilter.java:110)
    at org.springframework.security.web.context.SecurityContextPersistenceFilter.doFilter(SecurityContextPersistenceFilter.java:80)
    at org.springframework.security.web.FilterChainProxy$VirtualFilterChain.doFilter(FilterChainProxy.java:346)
    at org.springframework.web.filter.OncePerRequestFilter.doFilter(OncePerRequestFilter.java:102)
    at org.springframework.security.web.FilterChainProxy$VirtualFilterChain.doFilter(FilterChainProxy.java:346)
    at org.springframework.security.web.FilterChainProxy.doFilterInternal(FilterChainProxy.java:221)
    at org.springframework.security.web.FilterChainProxy.doFilter(FilterChainProxy.java:186)
    at org.springframework.web.filter.DelegatingFilterProxy.invokeDelegate(DelegatingFilterProxy.java:354)
    at org.springframework.web.filter.DelegatingFilterProxy.doFilter(DelegatingFilterProxy.java:267)
    at org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:178)
    at org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:153)
    at org.springframework.web.filter.RequestContextFilter.doFilterInternal(RequestContextFilter.java:100)
    at org.springframework.web.filter.OncePerRequestFilter.doFilter(OncePerRequestFilter.java:117)
    at org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:178)
    at org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:153)
    at org.springframework.web.filter.OncePerRequestFilter.doFilter(OncePerRequestFilter.java:102)
    at org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:178)
    at org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:153)
    at org.springframework.session.web.http.SessionRepositoryFilter.doFilterInternal(SessionRepositoryFilter.java:142)
    at org.springframework.session.web.http.OncePerRequestFilter.doFilter(OncePerRequestFilter.java:82)
    at org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:178)
    at org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:153)
    at org.springframework.web.filter.OncePerRequestFilter.doFilter(OncePerRequestFilter.java:102)
    at org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:178)
    at org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:153)
    at org.springframework.web.filter.OncePerRequestFilter.doFilter(OncePerRequestFilter.java:102)
    at org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:178)
    at org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:153)
    at org.apache.catalina.core.ApplicationDispatcher.invoke(ApplicationDispatcher.java:661)
    at org.apache.catalina.core.ApplicationDispatcher.processRequest(ApplicationDispatcher.java:427)
    at org.apache.catalina.core.ApplicationDispatcher.doForward(ApplicationDispatcher.java:357)
    at org.apache.catalina.core.ApplicationDispatcher.forward(ApplicationDispatcher.java:294)
    at org.apache.catalina.core.StandardHostValve.custom(StandardHostValve.java:377)
    at org.apache.catalina.core.StandardHostValve.status(StandardHostValve.java:237)
    at org.apache.catalina.core.StandardHostValve.invoke(StandardHostValve.java:166)
    at org.apache.catalina.valves.ErrorReportValve.invoke(ErrorReportValve.java:93)
    at org.apache.catalina.core.StandardEngineValve.invoke(StandardEngineValve.java:74)
    at org.apache.catalina.valves.RemoteIpValve.invoke(RemoteIpValve.java:765)
    at org.apache.catalina.connector.CoyoteAdapter.service(CoyoteAdapter.java:346)
    at org.apache.coyote.http11.Http11Processor.service(Http11Processor.java:390)
    at org.apache.coyote.AbstractProcessorLight.process(AbstractProcessorLight.java:63)
    at org.apache.coyote.AbstractProtocol$ConnectionHandler.process(AbstractProtocol.java:928)
    at org.apache.tomcat.util.net.NioEndpoint$SocketProcessor.doRun(NioEndpoint.java:1794)
    at org.apache.tomcat.util.net.SocketProcessorBase.run(SocketProcessorBase.java:52)
    at org.apache.tomcat.util.threads.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1191)
    at org.apache.tomcat.util.threads.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:659)
    at org.apache.tomcat.util.threads.TaskThread$WrappingRunnable.run(TaskThread.java:63)
    at java.base/java.lang.Thread.run(Thread.java:840)

The test uses basic auth, but we've seen this in production using oauth2.

* fix(core): remove ErrorPageSecurityFilter bean named errorPageSecurityInterceptor

to prevent java.lang.UnsupportedOperationException: public abstract int
javax.servlet.ServletRequest.getLocalPort() is not supported when processing error
responses.

See spring-projects/spring-security#11055 (comment) for background.

* refactor(basic): use constructor injection in BasicAuthConfig

to facilitate testing

* test(web): verify some error handling behavior of AuthConfig
  • Loading branch information
dbyron-sf authored Aug 10, 2024
1 parent 08f368e commit 401e717
Show file tree
Hide file tree
Showing 6 changed files with 363 additions and 3 deletions.
1 change: 1 addition & 0 deletions gate-basic/gate-basic.gradle
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
dependencies {
implementation project(":gate-core")
implementation "io.spinnaker.kork:kork-annotations"
implementation "io.spinnaker.kork:kork-security"
implementation "org.springframework.session:spring-session-core"
}
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@

import com.netflix.spinnaker.gate.config.AuthConfig;
import com.netflix.spinnaker.gate.security.SpinnakerAuthConfig;
import com.netflix.spinnaker.kork.annotations.VisibleForTesting;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.boot.autoconfigure.condition.ConditionalOnExpression;
import org.springframework.boot.autoconfigure.security.SecurityProperties;
Expand All @@ -35,16 +36,20 @@
@EnableWebSecurity
public class BasicAuthConfig extends WebSecurityConfigurerAdapter {

private final AuthConfig authConfig;
@VisibleForTesting protected final AuthConfig authConfig;

private final BasicAuthProvider authProvider;

@Autowired DefaultCookieSerializer defaultCookieSerializer;
@VisibleForTesting protected final DefaultCookieSerializer defaultCookieSerializer;

@Autowired
public BasicAuthConfig(AuthConfig authConfig, SecurityProperties securityProperties) {
public BasicAuthConfig(
AuthConfig authConfig,
SecurityProperties securityProperties,
DefaultCookieSerializer defaultCookieSerializer) {
this.authConfig = authConfig;
this.authProvider = new BasicAuthProvider(securityProperties);
this.defaultCookieSerializer = defaultCookieSerializer;
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,16 @@ public void configure(HttpSecurity http) throws Exception {
.authorizeRequests(
registry -> {
registry
// https://github.com/spring-projects/spring-security/issues/11055#issuecomment-1098061598 suggests
//
// filterSecurityInterceptorOncePerRequest(false)
//
// until spring boot 3.0. Since
//
// .antMatchers("/error").permitAll()
//
// permits unauthorized access to /error, filterSecurityInterceptorOncePerRequest
// isn't relevant.
.antMatchers("/error")
.permitAll()
.antMatchers("/favicon.ico")
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
package com.netflix.spinnaker.gate.config;

import org.springframework.beans.factory.annotation.Value;
import org.springframework.beans.factory.config.BeanFactoryPostProcessor;
import org.springframework.beans.factory.support.DefaultListableBeanFactory;
import org.springframework.boot.web.context.WebServerApplicationContext;
import org.springframework.boot.web.embedded.tomcat.TomcatWebServer;
import org.springframework.boot.web.server.WebServer;
Expand All @@ -19,6 +21,33 @@ public class MultiAuthSupport {
@Value("${default.legacy-server-port:-1}")
private int legacyServerPort;

/**
* From https://github.com/spring-projects/spring-security/issues/11055#issuecomment-1098061598,
* to fix java.lang.UnsupportedOperationException: public abstract int
* javax.servlet.ServletRequest.getLocalPort() is not supported when processing error responses
* for spring boot >= 2.6.4 and <= 3.0.0.
*
* <p>https://github.com/spring-projects/spring-boot/commit/71acc90da8 removed
* ErrorPageSecurityFilterConfiguration (which registered the errorPageSecurityInterceptor bean of
* type FilterRegistrationBean<ErrorPageSecurityFilter> for 2.7.x), but added
* ErrorPageSecurityFilterConfiguration to SpringBootWebSecurityConfiguration which registered a
* bean named errorPageSecurityFilter of the same type.
*
* <p>https://github.com/spring-projects/spring-boot/commit/4bd3534b7d91f922ad903a75beb19b6bdca39e5c
* reverted those changes for 3.0.0-M4 and 3.0.0-M5.
*
* <p>https://github.com/spring-projects/spring-boot/commit/cedd553b836d97a04d769322771bc1a8499e7282
* removed ErrorPageSecurityFilter and the corresponding filter for good in 3.0.0-RC1.
*
* <p>Deleting a bean by name fails if the bean doesn't exist.
*/
@Bean
public static BeanFactoryPostProcessor removeErrorSecurityFilter() {
return beanFactory ->
((DefaultListableBeanFactory) beanFactory)
.removeBeanDefinition("errorPageSecurityInterceptor");
}

@Bean
RequestMatcherProvider multiAuthRequestMatcherProvider(ApplicationContext applicationContext) {
return new RequestMatcherProvider() {
Expand Down
183 changes: 183 additions & 0 deletions gate-web/src/test/java/com/netflix/spinnaker/gate/AuthConfigTest.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,183 @@
/*
* Copyright 2024 Salesforce, Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package com.netflix.spinnaker.gate;

import static org.assertj.core.api.Assertions.assertThat;

import com.fasterxml.jackson.databind.ObjectMapper;
import com.netflix.spinnaker.gate.config.AuthConfig;
import com.netflix.spinnaker.gate.health.DownstreamServicesHealthIndicator;
import com.netflix.spinnaker.gate.security.basic.BasicAuthConfig;
import com.netflix.spinnaker.gate.services.ApplicationService;
import com.netflix.spinnaker.gate.services.DefaultProviderLookupService;
import java.io.IOException;
import java.util.Map;
import javax.servlet.http.HttpServletResponse;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.TestInfo;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.boot.autoconfigure.security.SecurityProperties;
import org.springframework.boot.test.context.SpringBootTest;
import org.springframework.boot.test.mock.mockito.MockBean;
import org.springframework.boot.test.web.client.TestRestTemplate;
import org.springframework.context.annotation.Bean;
import org.springframework.context.annotation.Configuration;
import org.springframework.context.annotation.Primary;
import org.springframework.core.ParameterizedTypeReference;
import org.springframework.http.HttpMethod;
import org.springframework.http.HttpStatus;
import org.springframework.http.ResponseEntity;
import org.springframework.security.config.annotation.web.builders.HttpSecurity;
import org.springframework.session.web.http.DefaultCookieSerializer;
import org.springframework.test.context.TestPropertySource;
import org.springframework.web.bind.annotation.GetMapping;
import org.springframework.web.bind.annotation.RestController;

/** AuthConfig is in gate-core, but is about matching http requests, so use gate-web to test it. */
@SpringBootTest(
classes = {Main.class, AuthConfigTest.TestConfiguration.class},
webEnvironment = SpringBootTest.WebEnvironment.RANDOM_PORT)
@TestPropertySource(
properties = {
"spring.config.location=classpath:gate-test.yml",
"spring.security.user.name=testuser",
"spring.security.user.password=testpassword",
"security.basicform.enabled=true"
})
class AuthConfigTest {

private static final String TEST_USER = "testuser";

private static final String TEST_PASSWORD = "testpassword";

private static final ParameterizedTypeReference<Map<String, String>> mapType =
new ParameterizedTypeReference<>() {};

@Autowired TestRestTemplate restTemplate;

@Autowired ObjectMapper objectMapper;

/** To prevent periodic calls to service's /health endpoints */
@MockBean DownstreamServicesHealthIndicator downstreamServicesHealthIndicator;

/** to prevent period application loading */
@MockBean ApplicationService applicationService;

/** To prevent attempts to load accounts */
@MockBean DefaultProviderLookupService defaultProviderLookupService;

@BeforeEach
void init(TestInfo testInfo) {
System.out.println("--------------- Test " + testInfo.getDisplayName());
}

@Test
void forwardNoCredsRequiresAuth() {
final ResponseEntity<Map<String, String>> response =
restTemplate.exchange("/forward", HttpMethod.GET, null, mapType);

// Without .antMatchers("/error").permitAll() in AuthConfig, we'd expect to
// get an empty error response since the request is unauthorized.
// https://github.com/spring-projects/spring-boot/issues/26356 has details.

// Leave this test here in case someone gets the urge to restrict access to /error.
assertThat(response.getStatusCode()).isEqualTo(HttpStatus.UNAUTHORIZED);
assertThat(response.getBody()).isNotNull();
assertThat(response.getBody().get("timestamp")).isNotNull();
assertThat(response.getBody().get("status"))
.isEqualTo(String.valueOf(HttpStatus.UNAUTHORIZED.value()));
assertThat(response.getBody().get("error"))
.isEqualTo(HttpStatus.UNAUTHORIZED.getReasonPhrase());
assertThat(response.getBody().get("message"))
.isEqualTo(HttpStatus.UNAUTHORIZED.getReasonPhrase());
}

@Test
void forwardWrongCredsRequiresAuth() {
final ResponseEntity<Map<String, String>> response =
restTemplate
.withBasicAuth(TEST_USER, "wrong" + TEST_PASSWORD)
.exchange("/forward", HttpMethod.GET, null, mapType);

assertThat(response.getStatusCode()).isEqualTo(HttpStatus.UNAUTHORIZED);
assertThat(response.getBody()).isNotNull();
assertThat(response.getBody().get("timestamp")).isNotNull();
assertThat(response.getBody().get("status"))
.isEqualTo(String.valueOf(HttpStatus.UNAUTHORIZED.value()));
assertThat(response.getBody().get("error"))
.isEqualTo(HttpStatus.UNAUTHORIZED.getReasonPhrase());
assertThat(response.getBody().get("message"))
.isEqualTo(HttpStatus.UNAUTHORIZED.getReasonPhrase());
}

@Test
void forwardWithCorrectCreds() {
final ResponseEntity<Object> response =
restTemplate
.withBasicAuth(TEST_USER, TEST_PASSWORD)
.exchange("/forward", HttpMethod.GET, null, Object.class);
assertThat(response.getStatusCode()).isEqualTo(HttpStatus.FOUND);
assertThat(response.getHeaders().getLocation().getPath()).isEqualTo("/hello");
assertThat(response.getBody()).isNull();
}

static class TestAuthConfig extends BasicAuthConfig {
public TestAuthConfig(
AuthConfig authConfig,
SecurityProperties securityProperties,
DefaultCookieSerializer defaultCookieSerializer) {
super(authConfig, securityProperties, defaultCookieSerializer);
}

@Override
protected void configure(HttpSecurity http) throws Exception {
// This is the same as BasicAuthConfig except for
//
// authenticationEntryPoint(new LoginUrlAuthenticationEntryPoint("/login"));
//
// Leaving that out makes it easier to test some behavior of AuthConfig.
defaultCookieSerializer.setSameSite(null);
http.formLogin().and().httpBasic();
authConfig.configure(http);
}
}

@Configuration
static class TestConfiguration {
@RestController
public static class TestController {
@GetMapping("/forward")
public void forward(HttpServletResponse response) throws IOException {
response.sendRedirect("/hello");
}

@GetMapping("/hello")
public String hello() {
return "hello";
}
}

@Bean
@Primary
BasicAuthConfig basicAuthConfig(
AuthConfig autoConfig,
SecurityProperties securityProperties,
DefaultCookieSerializer defaultCookieSerializer) {
return new TestAuthConfig(autoConfig, securityProperties, defaultCookieSerializer);
}
}
}
Loading

0 comments on commit 401e717

Please sign in to comment.