diff --git a/gate-basic/gate-basic.gradle b/gate-basic/gate-basic.gradle index a2b6bf2f86..42b2127af5 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 4816a70761..74f8e0f669 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,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 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 a1c68c287c..ee82c4312d 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-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 15b79854c4..2107746210 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,32 @@ 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("errorPageSecurityFilter"); + } + @Bean RequestMatcherProvider multiAuthRequestMatcherProvider(ApplicationContext applicationContext) { return new RequestMatcherProvider() { 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 0000000000..4934f114e3 --- /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); + } + } +} 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 0000000000..5c74539e2e --- /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. + 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(); + } +}