Merge pull request #1165 from Nike-Inc/feat/clarify-expected-path-err…

…or-CERB-2058

Return 404 instead of 500 for mismatched paths

cerberus-api-tests/src/integration-test/groovy/com/nike/cerberus/api/CerberusCompositeApiActions.groovy

@@ -48,6 +48,17 @@ class CerberusCompositeApiActions {
         "create, read, update then delete a secret node"(cerberusAuthToken, ROOT_INTEGRATION_TEST_SDB_PATH)
     }
 
+    static void "verify 404 with mismatched case in path"(String cerberusAuthToken, String sdbPath){
+        def path = sdbPath.toUpperCase()
+
+        try {
+            readSecretNode(path, cerberusAuthToken)
+        }
+        catch(java.lang.AssertionError e){
+            assertTrue(e.getMessage().contains("Expected status code <200> but was <404>"))
+        }
+    }
+
     static void "create, read, update then delete a secret node"(String cerberusAuthToken, String sdbPath) {
         def path = "${sdbPath}/${UUID.randomUUID().toString()}"
         String value1 = 'value1'

cerberus-api-tests/src/integration-test/groovy/com/nike/cerberus/api/CerberusIamApiV2Tests.groovy

@@ -79,6 +79,11 @@ class CerberusIamApiV2Tests {
         testPartition = PropUtils.getPropWithDefaultValue("TEST_PARTITION", "aws")
     }
 
+    @Test
+    void "test that a 404 is returned with mismatched case in path"() {
+        "verify 404 with mismatched case in path"(cerberusAuthToken, ROOT_INTEGRATION_TEST_SDB_PATH)
+    }
+
     @Test
     void "test that an authenticated IAM role can create, read, update then delete a secret node"() {
         "create, read, update then delete a secret node"(cerberusAuthToken)

cerberus-web/src/main/java/com/nike/cerberus/error/InvalidEncryptionContextApiError.java

@@ -0,0 +1,39 @@
+package com.nike.cerberus.error;
+
+import com.nike.backstopper.apierror.ApiError;
+import java.util.Map;
+import javax.servlet.http.HttpServletResponse;
+
+public class InvalidEncryptionContextApiError implements ApiError {
+
+  private final String message;
+
+  public InvalidEncryptionContextApiError(String message) {
+    this.message = message;
+  }
+
+  @Override
+  public String getName() {
+    return "InvalidEncryptionContextException";
+  }
+
+  @Override
+  public String getErrorCode() {
+    return DefaultApiError.ENTITY_NOT_FOUND.getErrorCode();
+  }
+
+  @Override
+  public String getMessage() {
+    return this.message;
+  }
+
+  @Override
+  public Map<String, Object> getMetadata() {
+    return null;
+  }
+
+  @Override
+  public int getHttpStatusCode() {
+    return HttpServletResponse.SC_NOT_FOUND;
+  }
+}

cerberus-web/src/main/java/com/nike/cerberus/service/EncryptionService.java

@@ -26,6 +26,8 @@
 import com.amazonaws.encryptionsdk.multi.MultipleProviderFactory;
 import com.amazonaws.regions.Region;
 import com.google.common.collect.Lists;
+import com.nike.backstopper.exception.ApiException;
+import com.nike.cerberus.error.InvalidEncryptionContextApiError;
 import com.nike.cerberus.util.CiphertextUtils;
 import java.nio.charset.StandardCharsets;
 import java.util.ArrayList;
@@ -232,7 +234,10 @@ public static String decrypt(
    *
    * <p>This step validates that the encrypted payload was created for the SDB that is currently
    * being decrypted. It is an integrity check. If this validation fails then the encrypted payload
-   * may have been tampered with, e.g. copying the encrypted payload between two SDBs.
+   * may have been tampered with, e.g. copying the encrypted payload between two SDBs. However,
+   * another possibility exists: there are times wherein the case of the path differs. In either
+   * case we still consider the validation to have failed, but we want to log the different case
+   * scenario specifically.
    *
    * @param parsedCiphertext the ciphertext to read the encryptionContext from
    * @param sdbPath the path expected in the encryptionContext
@@ -241,9 +246,15 @@ private void validateEncryptionContext(ParsedCiphertext parsedCiphertext, String
     Map<String, String> encryptionContext = parsedCiphertext.getEncryptionContextMap();
     String pathFromEncryptionContext = encryptionContext.getOrDefault(SDB_PATH_PROPERTY_NAME, null);
     if (!StringUtils.equals(pathFromEncryptionContext, sdbPath)) {
-      log.error("EncryptionContext did not have expected path, possible tampering: " + sdbPath);
-      throw new IllegalArgumentException(
-          "EncryptionContext did not have expected path, possible tampering: " + sdbPath);
+      String msg = "EncryptionContext did not have expected path: " + sdbPath;
+      if (StringUtils.equalsIgnoreCase(pathFromEncryptionContext, sdbPath)) {
+        msg += " (case path change detected)";
+      } else {
+        msg += " (possible tampering)";
+      }
+      log.error(msg);
+      InvalidEncryptionContextApiError iece = new InvalidEncryptionContextApiError(msg);
+      throw ApiException.newBuilder().withApiErrors(iece).build();
     }
   }
 

cerberus-web/src/test/java/com/nike/cerberus/service/EncryptionServiceTest.java

@@ -32,6 +32,7 @@
 import com.amazonaws.regions.Region;
 import com.amazonaws.regions.Regions;
 import com.google.common.collect.Lists;
+import com.nike.backstopper.exception.ApiException;
 import java.nio.charset.StandardCharsets;
 import java.util.HashMap;
 import java.util.List;
@@ -118,11 +119,32 @@ public void testDecryptWhenEncryptionContentDidNotHaveExpectedPath() {
     String exceptionMessage = "";
     try {
       encryptionService.decrypt("encryptedPayload", "sdbPath");
-    } catch (IllegalArgumentException illegalArgumentException) {
-      exceptionMessage = illegalArgumentException.getMessage();
+    } catch (ApiException apie) {
+      exceptionMessage = apie.getMessage();
+    }
+    assertEquals(
+        "EncryptionContext did not have expected path: sdbPath (possible tampering)",
+        exceptionMessage);
+  }
+
+  @Test
+  public void testDecryptWhenEncryptionContentDidNotHaveExpectedPathCase() {
+    EncryptionService encryptionService = Mockito.spy(getEncryptionService());
+    ParsedCiphertext parsedCiphertext = getParsedCipherText();
+    Mockito.doReturn(parsedCiphertext)
+        .when(encryptionService)
+        .getParsedCipherText("encryptedPayload");
+    Map<String, String> contextMap = new HashMap<>();
+    contextMap.put(SDB_PATH_PROPERTY_NAME, "sdbpath");
+    Mockito.when(parsedCiphertext.getEncryptionContextMap()).thenReturn(contextMap);
+    String exceptionMessage = "";
+    try {
+      encryptionService.decrypt("encryptedPayload", "SDBPath");
+    } catch (ApiException apie) {
+      exceptionMessage = apie.getMessage();
     }
     assertEquals(
-        "EncryptionContext did not have expected path, possible tampering: sdbPath",
+        "EncryptionContext did not have expected path: SDBPath (case path change detected)",
         exceptionMessage);
   }
 
@@ -157,11 +179,32 @@ public void testDecryptBytesWhenEncryptionContentDidNotHaveExpectedPath() {
     String exceptionMessage = "";
     try {
       encryptionService.decrypt("encryptedPayload".getBytes(StandardCharsets.UTF_8), "sdbPath");
-    } catch (IllegalArgumentException illegalArgumentException) {
-      exceptionMessage = illegalArgumentException.getMessage();
+    } catch (ApiException apie) {
+      exceptionMessage = apie.getMessage();
     }
     assertEquals(
-        "EncryptionContext did not have expected path, possible tampering: sdbPath",
+        "EncryptionContext did not have expected path: sdbPath (possible tampering)",
+        exceptionMessage);
+  }
+
+  @Test
+  public void testDecryptBytesWhenEncryptionContentDidNotHaveExpectedPathCase() {
+    EncryptionService encryptionService = Mockito.spy(getEncryptionService());
+    ParsedCiphertext parsedCiphertext = getParsedCipherText();
+    Mockito.doReturn(parsedCiphertext)
+        .when(encryptionService)
+        .getParsedCipherText("encryptedPayload".getBytes(StandardCharsets.UTF_8));
+    Map<String, String> contextMap = new HashMap<>();
+    contextMap.put(SDB_PATH_PROPERTY_NAME, "SdBpAtH");
+    Mockito.when(parsedCiphertext.getEncryptionContextMap()).thenReturn(contextMap);
+    String exceptionMessage = "";
+    try {
+      encryptionService.decrypt("encryptedPayload".getBytes(StandardCharsets.UTF_8), "sdbPath");
+    } catch (ApiException apie) {
+      exceptionMessage = apie.getMessage();
+    }
+    assertEquals(
+        "EncryptionContext did not have expected path: sdbPath (case path change detected)",
         exceptionMessage);
   }
 
@@ -197,11 +240,11 @@ public void testReEncryptWhenEncryptionContentDidNotHaveExpectedPath() {
     String exceptionMessage = "";
     try {
       encryptionService.reencrypt("encryptedPayload", "sdbPath");
-    } catch (IllegalArgumentException illegalArgumentException) {
-      exceptionMessage = illegalArgumentException.getMessage();
+    } catch (ApiException apie) {
+      exceptionMessage = apie.getMessage();
     }
     assertEquals(
-        "EncryptionContext did not have expected path, possible tampering: sdbPath",
+        "EncryptionContext did not have expected path: sdbPath (possible tampering)",
         exceptionMessage);
   }
 
@@ -270,11 +313,11 @@ public void testReEncryptBytesWhenEncryptionContentDidNotHaveExpectedPath() {
     String exceptionMessage = "";
     try {
       encryptionService.reencrypt("encryptedPayload".getBytes(StandardCharsets.UTF_8), "sdbPath");
-    } catch (IllegalArgumentException illegalArgumentException) {
-      exceptionMessage = illegalArgumentException.getMessage();
+    } catch (ApiException apie) {
+      exceptionMessage = apie.getMessage();
     }
     assertEquals(
-        "EncryptionContext did not have expected path, possible tampering: sdbPath",
+        "EncryptionContext did not have expected path: sdbPath (possible tampering)",
         exceptionMessage);
   }