Merge "Fix security issue in api interceptor"

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<String, String> 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);
 
@@ -194,12 +179,29 @@ public class ApiAuthInterceptionTest {
   }
 
   @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;
+  }
 }