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