From bff872b6ba2521447d8204717fee1eefc00dd654 Mon Sep 17 00:00:00 2001 From: Shaun Ford Date: Thu, 13 Jul 2017 11:01:14 -0700 Subject: [PATCH] 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. --- .../cerberus/service/KmsPolicyService.java | 33 +++++++++++++++---- .../com/nike/cerberus/service/KmsService.java | 8 +++-- .../service/KmsPolicyServiceTest.java | 30 +++++++++++++++++ 3 files changed, 63 insertions(+), 8 deletions(-) diff --git a/src/main/java/com/nike/cerberus/service/KmsPolicyService.java b/src/main/java/com/nike/cerberus/service/KmsPolicyService.java index af3f95d78..44cfcf0d8 100644 --- a/src/main/java/com/nike/cerberus/service/KmsPolicyService.java +++ b/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 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 existingStatements = policy.getStatements(); - List policyStatementsExcludingCMS = existingStatements.stream() - .filter(statement -> ! StringUtils.equals(statement.getId(), CERBERUS_MANAGEMENT_SERVICE_SID)) + List 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); } /** diff --git a/src/main/java/com/nike/cerberus/service/KmsService.java b/src/main/java/com/nike/cerberus/service/KmsService.java index 988d03a7c..b97ec2cfc 100644 --- a/src/main/java/com/nike/cerberus/service/KmsService.java +++ b/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); diff --git a/src/test/java/com/nike/cerberus/service/KmsPolicyServiceTest.java b/src/test/java/com/nike/cerberus/service/KmsPolicyServiceTest.java index 690f4db67..25249c051 100644 --- a/src/test/java/com/nike/cerberus/service/KmsPolicyServiceTest.java +++ b/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(); + } } \ No newline at end of file