From ba00a0d0f8d2fea8e39daf9e03100f7e8bf004d0 Mon Sep 17 00:00:00 2001 From: David Byron Date: Tue, 6 Aug 2024 14:32:46 -0700 Subject: [PATCH 1/5] test(web): demonstrate bug in MultiAutoSupport MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit where handling of certain error responses generates html: HTTP Status 400 – Bad Request

HTTP Status 400 – Bad Request

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. --- .../spinnaker/gate/MultiAuthSupportTest.java | 132 ++++++++++++++++++ 1 file changed, 132 insertions(+) create mode 100644 gate-web/src/test/java/com/netflix/spinnaker/gate/MultiAuthSupportTest.java diff --git a/gate-web/src/test/java/com/netflix/spinnaker/gate/MultiAuthSupportTest.java b/gate-web/src/test/java/com/netflix/spinnaker/gate/MultiAuthSupportTest.java new file mode 100644 index 000000000..e7e89232f --- /dev/null +++ b/gate-web/src/test/java/com/netflix/spinnaker/gate/MultiAuthSupportTest.java @@ -0,0 +1,132 @@ +/* + * 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 static org.mockito.Mockito.doThrow; +import static org.mockito.Mockito.verify; + +import com.fasterxml.jackson.databind.JsonNode; +import com.fasterxml.jackson.databind.ObjectMapper; +import com.netflix.spinnaker.gate.health.DownstreamServicesHealthIndicator; +import com.netflix.spinnaker.gate.services.ApplicationService; +import com.netflix.spinnaker.gate.services.DefaultProviderLookupService; +import com.netflix.spinnaker.gate.services.PipelineService; +import java.net.URI; +import java.net.http.HttpClient; +import java.net.http.HttpRequest; +import java.net.http.HttpResponse; +import java.nio.charset.StandardCharsets; +import java.util.Base64; +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.test.context.SpringBootTest; +import org.springframework.boot.test.mock.mockito.MockBean; +import org.springframework.boot.web.server.LocalServerPort; +import org.springframework.test.context.TestPropertySource; + +/** + * MultiAuthSupport is in gate-core, but is about matching http requests, so use code from gate-web + * to test it. + */ +@SpringBootTest(classes = Main.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 MultiAuthSupportTest { + + private static final String TEST_USER = "testuser"; + + private static final String TEST_PASSWORD = "testpassword"; + + @LocalServerPort private int port; + + @Autowired ObjectMapper objectMapper; + + @MockBean PipelineService pipelineService; + + /** 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 handleErrorResponse() throws Exception { + // given + String executionId = "12345"; + String expression = "arbitrary"; + + // mock an arbitrary endpoint to throw an exception + doThrow(new IllegalArgumentException("foo")) + .when(pipelineService) + .evaluateExpressionForExecution(executionId, expression); + + // when + String response = + callGate( + "http://localhost:" + + port + + "/pipelines/" + + executionId + + "/evaluateExpression?expression=" + + expression); + + // then + verify(pipelineService).evaluateExpressionForExecution(executionId, expression); + + assertThat(response).isNotNull(); + + // Validate that the response is json. FIXME: The response is HTML when things aren't working. + JsonNode json = objectMapper.readTree(response); + assertThat(json).isNotNull(); + } + + /** Generate a request to a gate endpoint that uses authorization and fails. */ + private String callGate(String url) throws Exception { + HttpClient client = HttpClient.newBuilder().build(); + + URI uri = new URI(url); + + String credentials = TEST_USER + ":" + TEST_PASSWORD; + byte[] encodedCredentials = + Base64.getEncoder().encode(credentials.getBytes(StandardCharsets.UTF_8)); + + HttpRequest request = + HttpRequest.newBuilder(uri) + .GET() + .header("Authorization", "Basic " + new String(encodedCredentials)) + .build(); + + HttpResponse response = client.send(request, HttpResponse.BodyHandlers.ofString()); + + return response.body(); + } +} From 26feb5668cd0c0dcbe44589d438155d68f64feec Mon Sep 17 00:00:00 2001 From: David Byron Date: Tue, 6 Aug 2024 16:41:16 -0700 Subject: [PATCH 2/5] 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 https://github.com/spring-projects/spring-security/issues/11055#issuecomment-1098061598 for background. --- .../gate/config/MultiAuthSupport.java | 29 +++++++++++++++++++ .../spinnaker/gate/MultiAuthSupportTest.java | 2 +- 2 files changed, 30 insertions(+), 1 deletion(-) diff --git a/gate-core/src/main/java/com/netflix/spinnaker/gate/config/MultiAuthSupport.java b/gate-core/src/main/java/com/netflix/spinnaker/gate/config/MultiAuthSupport.java index 15b79854c..29e651045 100644 --- a/gate-core/src/main/java/com/netflix/spinnaker/gate/config/MultiAuthSupport.java +++ b/gate-core/src/main/java/com/netflix/spinnaker/gate/config/MultiAuthSupport.java @@ -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; @@ -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. + * + *

https://github.com/spring-projects/spring-boot/commit/71acc90da8 removed + * ErrorPageSecurityFilterConfiguration (which registered the errorPageSecurityInterceptor bean of + * type FilterRegistrationBean for 2.7.x), but added + * ErrorPageSecurityFilterConfiguration to SpringBootWebSecurityConfiguration which registered a + * bean named errorPageSecurityFilter of the same type. + * + *

https://github.com/spring-projects/spring-boot/commit/4bd3534b7d91f922ad903a75beb19b6bdca39e5c + * reverted those changes for 3.0.0-M4 and 3.0.0-M5. + * + *

https://github.com/spring-projects/spring-boot/commit/cedd553b836d97a04d769322771bc1a8499e7282 + * removed ErrorPageSecurityFilter and the corresponding filter for good in 3.0.0-RC1. + * + *

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() { diff --git a/gate-web/src/test/java/com/netflix/spinnaker/gate/MultiAuthSupportTest.java b/gate-web/src/test/java/com/netflix/spinnaker/gate/MultiAuthSupportTest.java index e7e89232f..5c74539e2 100644 --- a/gate-web/src/test/java/com/netflix/spinnaker/gate/MultiAuthSupportTest.java +++ b/gate-web/src/test/java/com/netflix/spinnaker/gate/MultiAuthSupportTest.java @@ -104,7 +104,7 @@ void handleErrorResponse() throws Exception { assertThat(response).isNotNull(); - // Validate that the response is json. FIXME: The response is HTML when things aren't working. + // Validate that the response is json. JsonNode json = objectMapper.readTree(response); assertThat(json).isNotNull(); } From e57c64c49707712e2d29708d9d28d0f526ac34d8 Mon Sep 17 00:00:00 2001 From: David Byron Date: Tue, 6 Aug 2024 12:57:33 -0700 Subject: [PATCH 3/5] refactor(basic): use constructor injection in BasicAuthConfig to facilitate testing --- .../spinnaker/gate/security/basic/BasicAuthConfig.java | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/gate-basic/src/main/java/com/netflix/spinnaker/gate/security/basic/BasicAuthConfig.java b/gate-basic/src/main/java/com/netflix/spinnaker/gate/security/basic/BasicAuthConfig.java index 4816a7076..06280fb61 100644 --- a/gate-basic/src/main/java/com/netflix/spinnaker/gate/security/basic/BasicAuthConfig.java +++ b/gate-basic/src/main/java/com/netflix/spinnaker/gate/security/basic/BasicAuthConfig.java @@ -39,12 +39,16 @@ public class BasicAuthConfig extends WebSecurityConfigurerAdapter { private final BasicAuthProvider authProvider; - @Autowired DefaultCookieSerializer defaultCookieSerializer; + private 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 From 993bbc07d7ce2f83c08bfa1099bcc503803475d6 Mon Sep 17 00:00:00 2001 From: David Byron Date: Wed, 7 Aug 2024 18:32:07 -0700 Subject: [PATCH 4/5] test(web): verify some error handling behavior of AuthConfig --- gate-basic/gate-basic.gradle | 1 + .../gate/security/basic/BasicAuthConfig.java | 5 +- .../spinnaker/gate/config/AuthConfig.java | 10 + .../spinnaker/gate/AuthConfigTest.java | 183 ++++++++++++++++++ 4 files changed, 197 insertions(+), 2 deletions(-) create mode 100644 gate-web/src/test/java/com/netflix/spinnaker/gate/AuthConfigTest.java diff --git a/gate-basic/gate-basic.gradle b/gate-basic/gate-basic.gradle index a2b6bf2f8..42b2127af 100644 --- a/gate-basic/gate-basic.gradle +++ b/gate-basic/gate-basic.gradle @@ -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" } diff --git a/gate-basic/src/main/java/com/netflix/spinnaker/gate/security/basic/BasicAuthConfig.java b/gate-basic/src/main/java/com/netflix/spinnaker/gate/security/basic/BasicAuthConfig.java index 06280fb61..74f8e0f66 100644 --- a/gate-basic/src/main/java/com/netflix/spinnaker/gate/security/basic/BasicAuthConfig.java +++ b/gate-basic/src/main/java/com/netflix/spinnaker/gate/security/basic/BasicAuthConfig.java @@ -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; @@ -35,11 +36,11 @@ @EnableWebSecurity public class BasicAuthConfig extends WebSecurityConfigurerAdapter { - private final AuthConfig authConfig; + @VisibleForTesting protected final AuthConfig authConfig; private final BasicAuthProvider authProvider; - private final DefaultCookieSerializer defaultCookieSerializer; + @VisibleForTesting protected final DefaultCookieSerializer defaultCookieSerializer; @Autowired public BasicAuthConfig( diff --git a/gate-core/src/main/java/com/netflix/spinnaker/gate/config/AuthConfig.java b/gate-core/src/main/java/com/netflix/spinnaker/gate/config/AuthConfig.java index a1c68c287..ee82c4312 100644 --- a/gate-core/src/main/java/com/netflix/spinnaker/gate/config/AuthConfig.java +++ b/gate-core/src/main/java/com/netflix/spinnaker/gate/config/AuthConfig.java @@ -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") diff --git a/gate-web/src/test/java/com/netflix/spinnaker/gate/AuthConfigTest.java b/gate-web/src/test/java/com/netflix/spinnaker/gate/AuthConfigTest.java new file mode 100644 index 000000000..4934f114e --- /dev/null +++ b/gate-web/src/test/java/com/netflix/spinnaker/gate/AuthConfigTest.java @@ -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> 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> 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> 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 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); + } + } +} From 571c1e84735c26328bc0b6705ddac61f381022fc Mon Sep 17 00:00:00 2001 From: David Byron Date: Wed, 7 Aug 2024 07:06:21 -0700 Subject: [PATCH 5/5] fix(core): update the name of the ErrorPageSecurityFilter bean for spring boot 2.7.x --- .../com/netflix/spinnaker/gate/config/MultiAuthSupport.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/gate-core/src/main/java/com/netflix/spinnaker/gate/config/MultiAuthSupport.java b/gate-core/src/main/java/com/netflix/spinnaker/gate/config/MultiAuthSupport.java index 29e651045..210774621 100644 --- a/gate-core/src/main/java/com/netflix/spinnaker/gate/config/MultiAuthSupport.java +++ b/gate-core/src/main/java/com/netflix/spinnaker/gate/config/MultiAuthSupport.java @@ -44,8 +44,7 @@ public class MultiAuthSupport { @Bean public static BeanFactoryPostProcessor removeErrorSecurityFilter() { return beanFactory -> - ((DefaultListableBeanFactory) beanFactory) - .removeBeanDefinition("errorPageSecurityInterceptor"); + ((DefaultListableBeanFactory) beanFactory).removeBeanDefinition("errorPageSecurityFilter"); } @Bean