Fix issue where key policy is not valid at update

Fix the issue where cleanup fails to give permission for CMS to delete the inactive KMS key.

This is important because sometimes the consumer IAM principal gets deleted in AWS, but the KMS
key policy stays around, in which case, Amazon replaces the princnipal ARN with the princnipal 'ID'
behind the scenes.

If that key policy containing a principal 'ID' is ever updated then the update will fail because
a princnipal 'ID' is not considered to be a valid AWS principal.

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

@@ -24,6 +24,7 @@
 import com.amazonaws.auth.policy.Statement;
 import com.amazonaws.auth.policy.actions.KMSActions;
 import com.amazonaws.auth.policy.internal.JsonPolicyReader;
+import com.amazonaws.services.kms.model.PutKeyPolicyRequest;
 import org.apache.commons.lang3.StringUtils;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
@@ -85,7 +86,6 @@ public KmsPolicyService(@Named(ROOT_USER_ARN_PROPERTY) String rootUserArn,
      * @return true if the policy is valid, false if the policy contains an ID because the ARN had been deleted and recreated
      */
     public boolean isPolicyValid(String policyJson, String iamRoleArn) {
-
         return iamPrincipalCanAccessKey(policyJson, iamRoleArn) && cmsHasKeyDeletePermissions(policyJson);
     }
 
@@ -145,15 +145,36 @@ protected boolean cmsHasKeyDeletePermissions(String policyJson) {
      * @return - The updated JSON KMS policy containing a regenerated statement for CMS
      */
     protected String overwriteCMSPolicy(String policyJson) {
+        Policy policy = policyReader.createPolicyFromJsonString(policyJson);
+        removeStatementFromPolicy(policy, CERBERUS_MANAGEMENT_SERVICE_SID);
+        Collection<Statement> statements = policy.getStatements();
+        statements.add(generateStandardCMSPolicyStatement());
+        return policy.toJson();
+    }
 
+    /**
+     * Removes the 'Allow' statement for the consumer IAM principal.
+     *
+     * This is important when updating the KMS policy
+     * because if the IAM principal has been deleted then the KMS policy will contain the principal 'ID' instead of the
+     * ARN, which renders the policy invalid when calling {@link com.amazonaws.services.kms.AWSKMSClient#putKeyPolicy(PutKeyPolicyRequest)}.
+     *
+     * @param policyJson - Key policy JSON from which to remove consumer principal
+     * @return - The updated key policy JSON
+     */
+    protected String removeConsumerPrincipalFromPolicy(String policyJson) {
         Policy policy = policyReader.createPolicyFromJsonString(policyJson);
+        removeStatementFromPolicy(policy, CERBERUS_CONSUMER_SID);
+        return policy.toJson();
+    }
+
+    protected void removeStatementFromPolicy(Policy policy, String statementId) {
         Collection<Statement> existingStatements = policy.getStatements();
-        List<Statement> policyStatementsExcludingCMS = existingStatements.stream()
-                .filter(statement -> ! StringUtils.equals(statement.getId(), CERBERUS_MANAGEMENT_SERVICE_SID))
+        List<Statement> policyStatementsExcludingConsumer = existingStatements.stream()
+                .filter(statement -> ! StringUtils.equals(statement.getId(), statementId))
                 .collect(Collectors.toList());
-        policyStatementsExcludingCMS.add(generateStandardCMSPolicyStatement());
-        policy.setStatements(policyStatementsExcludingCMS);
-        return policy.toJson();
+        policyStatementsExcludingConsumer.add(generateStandardCMSPolicyStatement());
+        policy.setStatements(policyStatementsExcludingConsumer);
     }
 
     /**

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

@@ -239,11 +239,15 @@ protected void validatePolicyAllowsCMSToDeleteCMK(String awsKmsKeyArn, String km
             String policyJson = getKmsKeyPolicy(awsKmsKeyArn, kmsCMKRegion);
 
             if (!kmsPolicyService.cmsHasKeyDeletePermissions(policyJson)) {
-                // only overwrite the policy statement for CMS instead of regenerating the entire policy because regenerating
+                // Overwrite the policy statement for CMS only, instead of regenerating the entire policy because regenerating
                 // the full policy would require unnecessarily looking up the associated IAM principal in the DB
                 String updatedPolicy = kmsPolicyService.overwriteCMSPolicy(policyJson);
 
-                updateKmsKeyPolicy(updatedPolicy, awsKmsKeyArn, kmsCMKRegion);
+                // If the consumer IAM principal has been deleted then the policy will contain a principal 'ID' instead
+                // of and ARN, rendering the policy invalid. So delete the consumer statement here just in case
+                String updatedPolicyWithNoConsumer = kmsPolicyService.removeConsumerPrincipalFromPolicy(updatedPolicy);
+
+                updateKmsKeyPolicy(updatedPolicyWithNoConsumer, awsKmsKeyArn, kmsCMKRegion);
             }
         } catch (AmazonServiceException ase) {
             logger.error("Failed to validate that CMS can delete the given KMS key, ARN: {}, region: {}", awsKmsKeyArn, kmsCMKRegion, ase);

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

@@ -8,6 +8,7 @@
 import com.fasterxml.jackson.databind.ObjectMapper;
 import org.apache.commons.io.IOUtils;
 import org.apache.commons.lang3.StringUtils;
+import org.assertj.core.util.Lists;
 import org.junit.Before;
 import org.junit.Test;
 
@@ -160,4 +161,33 @@ public void test_that_generateStandardCMSPolicyStatement_returns_a_valid_stateme
         assertEquals(Statement.Effect.Allow, result.getEffect());
         assertTrue(kmsPolicyService.cmsHasKeyDeletePermissions(new Policy().withStatements(result).toJson()));
     }
+
+    @Test
+    public void test_that_removePolicyFromStatement_removes_the_given_statement() {
+
+        String removeId = "remove id";
+        String keepId = "keep id";
+        Statement statementToRemove = new Statement(Statement.Effect.Allow).withId(removeId).withActions(KMSActions.AllKMSActions);
+        Statement statementToKeep = new Statement(Statement.Effect.Deny).withId(keepId).withActions(KMSActions.AllKMSActions);
+        Policy policy = new Policy("policy", Lists.newArrayList(statementToKeep, statementToRemove));
+
+        kmsPolicyService.removeStatementFromPolicy(policy, removeId);
+
+        assertTrue(policy.getStatements().contains(statementToKeep));
+        assertFalse(policy.getStatements().contains(statementToRemove));
+
+    }
+
+    @Test
+    public void test_that_removeConsumerPrincipalFromPolicy_removes_cms_statement() throws IOException {
+        InputStream policy = getClass().getClassLoader()
+                .getResourceAsStream("com/nike/cerberus/service/valid-cerberus-iam-auth-kms-key-policy.json");
+        String policyJsonAsString = IOUtils.toString(policy, "UTF-8");
+
+        String result = kmsPolicyService.removeConsumerPrincipalFromPolicy(policyJsonAsString);
+        assertTrue(StringUtils.contains(policyJsonAsString, KmsPolicyService.CERBERUS_CONSUMER_SID));
+        assertFalse(StringUtils.contains(result, KmsPolicyService.CERBERUS_CONSUMER_SID));
+
+        policy.close();
+    }
 }