From db5f3e1fc72065397898bf5e8d1f03f3140600d0 Mon Sep 17 00:00:00 2001 From: Remigiusz Janeczek Date: Thu, 24 Oct 2019 08:50:49 +0200 Subject: Fix security issue in api interceptor Issue-ID: DCAEGEN2-1880 Change-Id: I5b93dd8405ef9a0a364c6e1224afcfacc9df1fba Signed-off-by: Remigiusz Janeczek --- .../org/onap/dcae/restapi/ApiAuthInterceptor.java | 14 ++++- .../onap/dcae/restapi/ApiAuthInterceptionTest.java | 73 +++++++++++++++------- 2 files changed, 61 insertions(+), 26 deletions(-) diff --git a/src/main/java/org/onap/dcae/restapi/ApiAuthInterceptor.java b/src/main/java/org/onap/dcae/restapi/ApiAuthInterceptor.java index a9281594..f1734080 100644 --- a/src/main/java/org/onap/dcae/restapi/ApiAuthInterceptor.java +++ b/src/main/java/org/onap/dcae/restapi/ApiAuthInterceptor.java @@ -55,8 +55,8 @@ public class ApiAuthInterceptor extends HandlerInterceptorAdapter { SubjectMatcher subjectMatcher = new SubjectMatcher(settings,(X509Certificate[]) request.getAttribute(CERTIFICATE_X_509)); - if(!settings.authMethod().equalsIgnoreCase(AuthMethodType.NO_AUTH.value()) && request.getServerPort() == settings.httpPort() ){ - if(request.getRequestURI().replaceAll("^/|/$", "").equalsIgnoreCase("healthcheck")){ + if(isHttpPortCalledWithAuthTurnedOn(request)){ + if(isHealthcheckCalledFromInsideCluster(request)){ return true; } response.getWriter().write("Operation not permitted"); @@ -78,6 +78,16 @@ public class ApiAuthInterceptor extends HandlerInterceptorAdapter { return true; } + private boolean isHttpPortCalledWithAuthTurnedOn(HttpServletRequest request) { + return !settings.authMethod().equalsIgnoreCase(AuthMethodType.NO_AUTH.value()) + && request.getLocalPort() == settings.httpPort(); + } + + private boolean isHealthcheckCalledFromInsideCluster(HttpServletRequest request) { + return request.getRequestURI().replaceAll("^/|/$", "").equalsIgnoreCase("healthcheck") + && request.getServerPort() == settings.httpPort(); + } + private boolean validateBasicHeader(HttpServletRequest request, HttpServletResponse response) throws IOException { String authorizationHeader = request.getHeader("Authorization"); diff --git a/src/test/java/org/onap/dcae/restapi/ApiAuthInterceptionTest.java b/src/test/java/org/onap/dcae/restapi/ApiAuthInterceptionTest.java index c80b56cb..250292f3 100644 --- a/src/test/java/org/onap/dcae/restapi/ApiAuthInterceptionTest.java +++ b/src/test/java/org/onap/dcae/restapi/ApiAuthInterceptionTest.java @@ -32,6 +32,7 @@ import org.onap.dcae.common.configuration.AuthMethodType; import org.slf4j.Logger; import org.springframework.http.HttpHeaders; import org.springframework.http.HttpStatus; +import org.springframework.mock.web.MockHttpServletRequest; import org.springframework.security.test.web.servlet.request.SecurityMockMvcRequestPostProcessors; import org.springframework.test.web.servlet.request.MockMvcRequestBuilders; @@ -50,6 +51,9 @@ public class ApiAuthInterceptionTest { private static final String USERNAME = "Foo"; private static final String PASSWORD = "Bar"; private static final Map CREDENTIALS = HashMap.of(USERNAME, PASSWORD); + private static final int HTTP_PORT = 8080; + private static final int OUTSIDE_PORT = 30235; + public static final String HEALTHCHECK_URL = "/healthcheck"; @Mock private Logger log; @@ -70,21 +74,6 @@ public class ApiAuthInterceptionTest { private ApiAuthInterceptor sut; - private HttpServletRequest createEmptyRequest() { - return MockMvcRequestBuilders - .post("") - .buildRequest(null); - } - - private HttpServletRequest createRequestWithAuthorizationHeader() { - return SecurityMockMvcRequestPostProcessors - .httpBasic(USERNAME, PASSWORD) - .postProcessRequest( - MockMvcRequestBuilders - .post("") - .buildRequest(null)); - } - @Test public void shouldSucceedWhenAuthorizationIsDisabled() throws IOException { // given @@ -176,16 +165,12 @@ public class ApiAuthInterceptionTest { } @Test - public void shouldSucceedForHealthcheckOnHealthcheckPort() throws IOException { + public void shouldSucceedForHealthcheckOnHealthcheckPortWhenRequestFromInsideCluster() throws IOException { // given - final HttpServletRequest request = - MockMvcRequestBuilders - .get("/healthcheck") - .buildRequest(null); + final HttpServletRequest request = createRequestWithPorts(HTTP_PORT, HTTP_PORT, HEALTHCHECK_URL); when(settings.authMethod()).thenReturn(AuthMethodType.CERT_BASIC_AUTH.value()); - when(settings.httpPort()).thenReturn(request.getServerPort()); - + when(settings.httpPort()).thenReturn(HTTP_PORT); // when final boolean isAuthorized = sut.preHandle(request, response, obj); @@ -193,13 +178,30 @@ public class ApiAuthInterceptionTest { assertTrue(isAuthorized); } + @Test + public void shouldFailForHealthcheckOnHealthcheckPortWhenRequestFromOutsideCluster() throws IOException { + // given + final HttpServletRequest request = createRequestWithPorts(HTTP_PORT, OUTSIDE_PORT, HEALTHCHECK_URL); + + when(settings.authMethod()).thenReturn(AuthMethodType.CERT_BASIC_AUTH.value()); + when(settings.httpPort()).thenReturn(HTTP_PORT); + when(response.getWriter()).thenReturn(writer); + + // when + final boolean isAuthorized = sut.preHandle(request, response, obj); + + // then + assertFalse(isAuthorized); + verify(response).setStatus(HttpStatus.BAD_REQUEST.value()); + } + @Test public void shouldFailDueToNotPermittedOperationOnHealthcheckPort() throws IOException { // given - final HttpServletRequest request = createEmptyRequest(); + final HttpServletRequest request = createRequestWithPorts(HTTP_PORT, HTTP_PORT, "/"); when(settings.authMethod()).thenReturn(AuthMethodType.CERT_BASIC_AUTH.value()); - when(settings.httpPort()).thenReturn(request.getServerPort()); + when(settings.httpPort()).thenReturn(HTTP_PORT); when(response.getWriter()).thenReturn(writer); // when @@ -210,4 +212,27 @@ public class ApiAuthInterceptionTest { verify(response).setStatus(HttpStatus.BAD_REQUEST.value()); } + private HttpServletRequest createEmptyRequest() { + return MockMvcRequestBuilders + .post("") + .buildRequest(null); + } + + private HttpServletRequest createRequestWithAuthorizationHeader() { + return SecurityMockMvcRequestPostProcessors + .httpBasic(USERNAME, PASSWORD) + .postProcessRequest( + MockMvcRequestBuilders + .post("") + .buildRequest(null)); + } + + private HttpServletRequest createRequestWithPorts(int localPort, int serverPort, String urlTemplate) { + MockHttpServletRequest healthcheckRequest = MockMvcRequestBuilders + .get(urlTemplate) + .buildRequest(null); + healthcheckRequest.setLocalPort(localPort); + healthcheckRequest.setServerPort(serverPort); + return healthcheckRequest; + } } -- cgit 1.2.3-korg