From 86a5c1c7d809850c1b3e2e79cccc3971635bd998 Mon Sep 17 00:00:00 2001 From: "John Haren (Nike)" Date: Mon, 15 May 2023 13:56:08 -0700 Subject: [PATCH] Return 404 instead of 500 for mismatched paths --- .../api/CerberusCompositeApiActions.groovy | 11 +++ .../cerberus/api/CerberusIamApiV2Tests.groovy | 5 ++ .../InvalidEncryptionContextApiError.java | 39 +++++++++++ .../cerberus/service/EncryptionService.java | 19 ++++-- .../service/EncryptionServiceTest.java | 67 +++++++++++++++---- 5 files changed, 125 insertions(+), 16 deletions(-) create mode 100644 cerberus-web/src/main/java/com/nike/cerberus/error/InvalidEncryptionContextApiError.java diff --git a/cerberus-api-tests/src/integration-test/groovy/com/nike/cerberus/api/CerberusCompositeApiActions.groovy b/cerberus-api-tests/src/integration-test/groovy/com/nike/cerberus/api/CerberusCompositeApiActions.groovy index f7b547ecf..838f6dcc4 100644 --- a/cerberus-api-tests/src/integration-test/groovy/com/nike/cerberus/api/CerberusCompositeApiActions.groovy +++ b/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' diff --git a/cerberus-api-tests/src/integration-test/groovy/com/nike/cerberus/api/CerberusIamApiV2Tests.groovy b/cerberus-api-tests/src/integration-test/groovy/com/nike/cerberus/api/CerberusIamApiV2Tests.groovy index 606646693..e33a59607 100644 --- a/cerberus-api-tests/src/integration-test/groovy/com/nike/cerberus/api/CerberusIamApiV2Tests.groovy +++ b/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) diff --git a/cerberus-web/src/main/java/com/nike/cerberus/error/InvalidEncryptionContextApiError.java b/cerberus-web/src/main/java/com/nike/cerberus/error/InvalidEncryptionContextApiError.java new file mode 100644 index 000000000..c1d9f08a4 --- /dev/null +++ b/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 getMetadata() { + return null; + } + + @Override + public int getHttpStatusCode() { + return HttpServletResponse.SC_NOT_FOUND; + } +} diff --git a/cerberus-web/src/main/java/com/nike/cerberus/service/EncryptionService.java b/cerberus-web/src/main/java/com/nike/cerberus/service/EncryptionService.java index b791ce2e4..c1a672db5 100644 --- a/cerberus-web/src/main/java/com/nike/cerberus/service/EncryptionService.java +++ b/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( * *

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 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(); } } diff --git a/cerberus-web/src/test/java/com/nike/cerberus/service/EncryptionServiceTest.java b/cerberus-web/src/test/java/com/nike/cerberus/service/EncryptionServiceTest.java index ddc939796..e633b06f1 100644 --- a/cerberus-web/src/test/java/com/nike/cerberus/service/EncryptionServiceTest.java +++ b/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 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 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); }