Merge pull request #54 from tlisonbee/update-key-validation

Moving key validation to avoid being rate limited by AWS

src/main/java/com/nike/cerberus/error/DefaultApiError.java

@@ -232,6 +232,11 @@ public enum DefaultApiError implements ApiError {
      */
     IAM_PRINCIPALS_CANNOT_USE_USER_ONLY_RESOURCE(99231, "The requested resource is for User Principals only.", HttpServletResponse.SC_FORBIDDEN),
 
+    /**
+     * KMS key is scheduled for deletion or disabled
+     */
+    KMS_KEY_IS_SCHEDULED_FOR_DELETION_OR_DISABLED(99232, "KMS key is scheduled for deletion or disabled", HttpServletResponse.SC_INTERNAL_SERVER_ERROR),
+
     /**
      * Generic not found error.
      */

src/main/java/com/nike/cerberus/service/AuthenticationService.java

@@ -497,8 +497,7 @@ protected String getKeyId(IamPrincipalCredentials credentials) {
         } else {
             kmsKeyRecord = kmsKey.get();
             kmsKeyId = kmsKeyRecord.getAwsKmsKeyId();
-            kmsService.validatePolicy(kmsKeyRecord, iamPrincipalArn);
-            kmsService.validateKmsKeyIsUsable(kmsKeyRecord, iamPrincipalArn);
+            kmsService.validateKeyAndPolicy(kmsKeyRecord, iamPrincipalArn);
         }
 
         return kmsKeyId;

src/main/java/com/nike/cerberus/service/KmsService.java

@@ -186,6 +186,8 @@ protected String getAliasName(String awsIamRoleKmsKeyId) {
     }
 
     /**
+     * Perform validation and fix some issues.
+     *
      * When a KMS key policy statement is created and an AWS ARN is specified as a principal,
      * AWS behind the scene binds that ARNs ID to the statement and not the ARN.
      *
@@ -201,9 +203,10 @@ protected String getAliasName(String awsIamRoleKmsKeyId) {
      * @param kmsKeyRecord - The CMK record to validate policy on
      * @param iamPrincipalArn - The principal ARN that should have decrypt permission
      */
-    public void validatePolicy(AwsIamRoleKmsKeyRecord kmsKeyRecord, String iamPrincipalArn) {
+    public void validateKeyAndPolicy(AwsIamRoleKmsKeyRecord kmsKeyRecord, String iamPrincipalArn) {
 
         if (! kmsPolicyNeedsValidation(kmsKeyRecord)) {
+            // Avoiding extra calls to AWS so that we don't get rate limited.
             return;
         }
 
@@ -221,6 +224,8 @@ public void validatePolicy(AwsIamRoleKmsKeyRecord kmsKeyRecord, String iamPrinci
                 updateKmsKeyPolicy(updatedPolicy, awsKmsKeyArn, kmsCMKRegion);
             }
 
+            validateKmsKeyIsUsable(kmsKeyRecord, iamPrincipalArn);
+
             // update last validated timestamp
             OffsetDateTime now = dateTimeSupplier.get();
             updateKmsKey(kmsKeyRecord.getAwsIamRoleId(), kmsCMKRegion, SYSTEM_USER, now, now);
@@ -281,10 +286,14 @@ protected void validateKmsKeyIsUsable(AwsIamRoleKmsKeyRecord kmsKeyRecord, Strin
         String awsKmsKeyArn = kmsKeyRecord.getAwsKmsKeyId();
 
         if (kmsKeyIsDisabledOrScheduledForDeletion(awsKmsKeyArn, kmsCMKRegion)) {
+            // This shouldn't happen unless there is a bug in CMS or if someone modified the key outside of Cerberus
             logger.warn("The KMS key ID: {} for IAM principal: {} is disabled or scheduled for deletion. Deleting the" +
                             "key record to prevent this from failing again: keyId: {} for IAM principal: {} in region: {}.",
                     awsKmsKeyArn, iamPrincipalArn, kmsCMKRegion);
             deleteKmsKeyById(kmsKeyRecord.getId());
+            throw ApiException.newBuilder()
+                    .withApiErrors(DefaultApiError.KMS_KEY_IS_SCHEDULED_FOR_DELETION_OR_DISABLED)
+                    .build();
         }
     }
 

src/test/java/com/nike/cerberus/service/AuthenticationServiceTest.java

@@ -169,7 +169,7 @@ public void test_that_getKeyId_only_validates_kms_policy_one_time_within_interva
 
         // verify validate is called once interval has passed
         assertEquals(cmkId, result);
-        verify(kmsService, times(1)).validatePolicy(awsIamRoleKmsKeyRecord, principalArn);
+        verify(kmsService, times(1)).validateKeyAndPolicy(awsIamRoleKmsKeyRecord, principalArn);
     }
 
     @Test

src/test/java/com/nike/cerberus/service/KmsServiceTest.java

@@ -125,6 +125,12 @@ public void test_validatePolicy_validates_policy_when_validate_interval_has_pass
         OffsetDateTime now = OffsetDateTime.now();
 
         AWSKMSClient client = mock(AWSKMSClient.class);
+        when(client.describeKey(anyObject())).thenReturn(
+                new DescribeKeyResult()
+                        .withKeyMetadata(
+                                new KeyMetadata()
+                                        .withKeyState(KeyState.Enabled)));
+
         when(kmsClientFactory.getClient(kmsCMKRegion)).thenReturn(client);
 
         GetKeyPolicyResult result = mock(GetKeyPolicyResult.class);
@@ -141,15 +147,15 @@ public void test_validatePolicy_validates_policy_when_validate_interval_has_pass
         when(awsIamRoleDao.getKmsKey(awsIamRoleRecordId, kmsCMKRegion)).thenReturn(Optional.of(kmsKey));
 
         when(dateTimeSupplier.get()).thenReturn(now);
-        kmsService.validatePolicy(kmsKey, kmsKeyArn);
+        kmsService.validateKeyAndPolicy(kmsKey, kmsKeyArn);
 
         verify(client, times(1)).getKeyPolicy(new GetKeyPolicyRequest().withKeyId(kmsKeyArn)
                 .withPolicyName("default"));
         verify(kmsPolicyService, times(1)).isPolicyValid(policy, kmsKeyArn);
     }
 
     @Test
-    public void test_validatePolicy_validates_policy_when_validate_interval_has_not_passed() {
+    public void test_validateKeyAndPolicy_validates_policy_when_validate_interval_has_not_passed() {
         String awsKmsKeyArn = "aws kms key arn";
         String iamPrincipalArn = "arn";
         String awsIamRoleRecordId = "aws iam role record id";
@@ -163,14 +169,14 @@ public void test_validatePolicy_validates_policy_when_validate_interval_has_not_
         when(kmsKey.getLastValidatedTs()).thenReturn(now);
 
         when(dateTimeSupplier.get()).thenReturn(now);
-        kmsService.validatePolicy(kmsKey, iamPrincipalArn);
+        kmsService.validateKeyAndPolicy(kmsKey, iamPrincipalArn);
 
         verify(kmsClientFactory, never()).getClient(anyString());
         verify(kmsPolicyService, never()).isPolicyValid(anyString(), anyString());
     }
 
     @Test
-    public void test_validatePolicy_does_not_throw_error_when_cannot_validate() {
+    public void test_validateKeyAndPolicy_does_not_throw_error_when_cannot_validate() {
         String keyId = "key-id";
         String iamPrincipalArn = "arn";
         String kmsCMKRegion = "kmsCMKRegion";
@@ -193,7 +199,7 @@ public void test_validatePolicy_does_not_throw_error_when_cannot_validate() {
         when(result.getPolicy()).thenReturn(policy);
         when(client.getKeyPolicy(new GetKeyPolicyRequest().withKeyId(keyId).withPolicyName("default"))).thenThrow(AmazonServiceException.class);
 
-        kmsService.validatePolicy(kmsKey, iamPrincipalArn);
+        kmsService.validateKeyAndPolicy(kmsKey, iamPrincipalArn);
 
         verify(kmsPolicyService, never()).isPolicyValid(policy, iamPrincipalArn);
         verify(client, never()).putKeyPolicy(anyObject());
@@ -311,7 +317,7 @@ public void test_validateKmsKeyIsUsable_returns_false_when_state_is_not_deletion
         assertFalse(result);
     }
 
-    @Test
+    @Test(expected = ApiException.class)
     public void test_validateKmsKeyIsUsable_deletes_kms_key_when_not_usable() {
 
         String id = "id";
@@ -333,8 +339,6 @@ public void test_validateKmsKeyIsUsable_deletes_kms_key_when_not_usable() {
                                         .withKeyState(KeyState.PendingDeletion)));
 
         kmsService.validateKmsKeyIsUsable(kmsKey, iamPrincipalArn);
-
-        verify(awsIamRoleDao, times(1)).deleteKmsKeyById(id);
     }
 
     @Test