From b17254486d8e20c44870906656352ab47b8bba2c Mon Sep 17 00:00:00 2001 From: Shaun Ford Date: Tue, 11 Jul 2017 18:05:18 -0700 Subject: [PATCH 1/4] Give CMS perms to schedule+cancel KMS key deletion * Updates the AWS SDK from 1.11.108 to 1.11.160 * Uses KMS Policy.toJson() + Policy.fromJson methods * Retroactively updates KMS key policies to include new perms for CMS --- gradle.properties | 2 +- gradle/dependencies.gradle | 2 +- .../nike/cerberus/service/CleanUpService.java | 12 +- .../cerberus/service/KmsPolicyService.java | 192 ++++++++++++------ .../com/nike/cerberus/service/KmsService.java | 69 ++++++- .../service/KmsPolicyServiceTest.java | 95 ++++++++- ...alid-cerberus-iam-auth-kms-key-policy.json | 3 +- ...erus-kms-key-policy-cms-cannot-delete.json | 61 ++++++ ...alid-cerberus-iam-auth-kms-key-policy.json | 11 +- 9 files changed, 360 insertions(+), 87 deletions(-) create mode 100644 src/test/resources/com/nike/cerberus/service/invalid-cerberus-kms-key-policy-cms-cannot-delete.json diff --git a/gradle.properties b/gradle.properties index c578207b6..f22a95ca1 100644 --- a/gradle.properties +++ b/gradle.properties @@ -14,6 +14,6 @@ # limitations under the License. # -version=0.22.0 +version=0.24.0 groupId=com.nike.cerberus artifactId=cms diff --git a/gradle/dependencies.gradle b/gradle/dependencies.gradle index e31822f3c..738a25234 100644 --- a/gradle/dependencies.gradle +++ b/gradle/dependencies.gradle @@ -23,7 +23,7 @@ def riposteVersion = '0.9.4' def logbackVersion = '1.1.7' // Request version 4 for full java 8 support def guiceVersion = '4.0' -def awsSdkVersion = '1.11.108' +def awsSdkVersion = '1.11.160' def groovyVersion = '2.3.9' dependencies { diff --git a/src/main/java/com/nike/cerberus/service/CleanUpService.java b/src/main/java/com/nike/cerberus/service/CleanUpService.java index 76fb9b0de..aa2f5e789 100644 --- a/src/main/java/com/nike/cerberus/service/CleanUpService.java +++ b/src/main/java/com/nike/cerberus/service/CleanUpService.java @@ -81,17 +81,17 @@ protected void cleanUpInactiveAndOrphanedKmsKeys(final int kmsKeysInactiveAfterN if (inactiveAndOrphanedKmsKeys.isEmpty()) { logger.info("No keys to clean up."); } else { - // delete inactive and orphaned kms key records from DB logger.info("Cleaning up orphaned or inactive KMS keys..."); inactiveAndOrphanedKmsKeys.forEach(kmsKeyRecord -> { + final String kmsKeyArn = kmsKeyRecord.getAwsKmsKeyId(); + final String kmsKeyRegion = kmsKeyRecord.getAwsRegion(); try { logger.info("Deleting orphaned or inactive KMS key: id={}, region={}, lastValidated={}", - kmsKeyRecord.getAwsKmsKeyId(), kmsKeyRecord.getAwsRegion(), kmsKeyRecord.getLastValidatedTs()); - - kmsService.scheduleKmsKeyDeletion(kmsKeyRecord.getAwsKmsKeyId(), kmsKeyRecord.getAwsRegion(), SOONEST_A_KMS_KEY_CAN_BE_DELETED); + kmsKeyArn, kmsKeyRegion, kmsKeyRecord.getLastValidatedTs()); + kmsService.validatePolicyAllowsCMSToDeleteCMK(kmsKeyArn, kmsKeyRegion); + kmsService.scheduleKmsKeyDeletion(kmsKeyArn, kmsKeyRegion, SOONEST_A_KMS_KEY_CAN_BE_DELETED); awsIamRoleDao.deleteKmsKeyById(kmsKeyRecord.getId()); - TimeUnit.SECONDS.sleep(sleepInSeconds); } catch (InterruptedException ie) { logger.error("Timeout between KMS key deletion was interrupted", ie); @@ -99,7 +99,7 @@ protected void cleanUpInactiveAndOrphanedKmsKeys(final int kmsKeysInactiveAfterN } catch (Exception e) { logger.error("There was a problem deleting KMS key with id: {}, region: {}", kmsKeyRecord.getAwsIamRoleId(), - kmsKeyRecord.getAwsRegion(), + kmsKeyRegion, e); } }); diff --git a/src/main/java/com/nike/cerberus/service/KmsPolicyService.java b/src/main/java/com/nike/cerberus/service/KmsPolicyService.java index 98a3e2418..af3f95d78 100644 --- a/src/main/java/com/nike/cerberus/service/KmsPolicyService.java +++ b/src/main/java/com/nike/cerberus/service/KmsPolicyService.java @@ -18,17 +18,22 @@ import com.amazonaws.auth.policy.Action; import com.amazonaws.auth.policy.Policy; +import com.amazonaws.auth.policy.PolicyReaderOptions; import com.amazonaws.auth.policy.Principal; import com.amazonaws.auth.policy.Resource; import com.amazonaws.auth.policy.Statement; -import com.fasterxml.jackson.databind.JsonNode; -import com.fasterxml.jackson.databind.ObjectMapper; +import com.amazonaws.auth.policy.actions.KMSActions; +import com.amazonaws.auth.policy.internal.JsonPolicyReader; +import org.apache.commons.lang3.StringUtils; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import javax.inject.Inject; import javax.inject.Named; import javax.inject.Singleton; +import java.util.Collection; +import java.util.List; +import java.util.stream.Collectors; /** * Helpful service for putting together the KMS policy documents to be associated with provisioned KMS keys. @@ -45,15 +50,18 @@ public class KmsPolicyService { private static final String CMS_ROLE_ARN_PROPERTY = "cms.role.arn"; private static final String AWS_PROVIDER = "AWS"; + public static final String CERBERUS_CONSUMER_SID = "Target IAM Role Has Decrypt Action"; + protected static final String CERBERUS_MANAGEMENT_SERVICE_SID = "CMS Role Key Access"; + private final String rootUserArn; private final String adminRoleArn; private final String cmsRoleArn; - private final ObjectMapper objectMapper; + private final JsonPolicyReader policyReader; @Inject public KmsPolicyService(@Named(ROOT_USER_ARN_PROPERTY) String rootUserArn, @@ -63,35 +71,42 @@ public KmsPolicyService(@Named(ROOT_USER_ARN_PROPERTY) String rootUserArn, this.adminRoleArn = adminRoleArn; this.cmsRoleArn = cmsRoleArn; - objectMapper = new ObjectMapper(); + PolicyReaderOptions policyReaderOptions = new PolicyReaderOptions(); + policyReaderOptions.setStripAwsPrincipalIdHyphensEnabled(false); + policyReader = new JsonPolicyReader(policyReaderOptions); } - /*** - * Please note that currently in the AWS Core SDK 1.11.108 that Policy.fromJson strips hyphens from AWS ARN Principals - * and Hyphens are valid in IAM role names. We will need to manually use JsonNodes and not rely on fromJson - * - * When you manually instantiate a Principal you can specify true/false for striping hyphens, - * when deserializing with fromJson this seems to always get set to true. + /** + * Validates that the given key policy grants appropriate permissions to CMS and allows the given IAM principal to + * access the KMS key. * - * @param policyString - The KMS key policy as a String + * @param policyJson - The KMS key policy as a String * @param iamRoleArn - The IAM Role that is supposed to have decrypt permissions * @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 policyString, String iamRoleArn) { - // The below json node stuff is lame, should be able to use Objects created from Policy.fromJson(string) - // todo file Github issue and or PR with AWS SDK project + public boolean isPolicyValid(String policyJson, String iamRoleArn) { + + return iamPrincipalCanAccessKey(policyJson, iamRoleArn) && cmsHasKeyDeletePermissions(policyJson); + } + + /** + * Check that the given IAM principal has permissions to access the KMS key. + * + * This is important because when an IAM principal is deleted and recreated with the same name, then the recreated + * principal cannot access the KMS key until the key policy is regenerated -- updating the policy permissions to + * allow the ARN of the recreated principal instead of the ID of the deleted principal. + * + * @param policyJson - The KMS key policy as a String + * @param iamPrincipalArn - The IAM Role that is supposed to have decrypt permissions + */ + protected boolean iamPrincipalCanAccessKey(String policyJson, String iamPrincipalArn) { try { - JsonNode policy = null; - policy = objectMapper.readTree(policyString); - JsonNode statements = policy.get("Statement"); - for (JsonNode statement : statements) { - if (CERBERUS_CONSUMER_SID.equals(statement.get("Sid").textValue())) { - String statementAWSPrincipal = statement.get("Principal").get("AWS").textValue(); - if (iamRoleArn.equals(statementAWSPrincipal)) { - return true; - } - } - } + Policy policy = policyReader.createPolicyFromJsonString(policyJson); + return policy.getStatements() + .stream() + .anyMatch(statement -> + StringUtils.equals(statement.getId(), CERBERUS_CONSUMER_SID) && + statementAppliesToPrincipal(statement, iamPrincipalArn)); } catch (Exception e) { // if we can't deserialize we will assume policy has been corrupted manually and regenerate it logger.error("Failed to validate policy, did someone manually edit the kms policy?", e); @@ -100,36 +115,114 @@ public boolean isPolicyValid(String policyString, String iamRoleArn) { return false; } + /** + * Validate that the IAM principal for the CMS has permissions to schedule and cancel deletion of the KMS key. + * @param policyJson - The KMS key policy as a String + */ + protected boolean cmsHasKeyDeletePermissions(String policyJson) { + try { + Policy policy = policyReader.createPolicyFromJsonString(policyJson); + return policy.getStatements() + .stream() + .anyMatch(statement -> + StringUtils.equals(statement.getId(), CERBERUS_MANAGEMENT_SERVICE_SID) && + statementAppliesToPrincipal(statement, cmsRoleArn) && + statement.getEffect() == Statement.Effect.Allow && + statementIncludesAction(statement, KMSActions.ScheduleKeyDeletion) && + statementIncludesAction(statement, KMSActions.CancelKeyDeletion)); + } catch (Exception e) { + logger.error("Failed to validate that CMS can delete KMS key, there may be something wrong with the policy", e); + } + + return false; + } + + /** + * Overwrite the policy statement for CMS with the standard statement. Add the standard statement for CMS + * to the policy if it did not already exist. + * + * @param policyJson - The KMS key policy in JSON format + * @return - The updated JSON KMS policy containing a regenerated statement for CMS + */ + protected String overwriteCMSPolicy(String policyJson) { + + Policy policy = policyReader.createPolicyFromJsonString(policyJson); + Collection existingStatements = policy.getStatements(); + List policyStatementsExcludingCMS = existingStatements.stream() + .filter(statement -> ! StringUtils.equals(statement.getId(), CERBERUS_MANAGEMENT_SERVICE_SID)) + .collect(Collectors.toList()); + policyStatementsExcludingCMS.add(generateStandardCMSPolicyStatement()); + policy.setStatements(policyStatementsExcludingCMS); + return policy.toJson(); + } + + /** + * Validates that the given KMS key policy statement applies to the given principal + */ + protected boolean statementAppliesToPrincipal(Statement statement, String principalArn) { + + return statement.getPrincipals() + .stream() + .anyMatch(principal -> + StringUtils.equals(principal.getId(), principalArn)); + } + + /** + * Validates that the given KMS key policy statement includes the given action + */ + protected boolean statementIncludesAction(Statement statement, Action action) { + + return statement.getActions() + .stream() + .anyMatch(statementAction -> + StringUtils.equals(statementAction.getActionName(), action.getActionName())); + } + + /** + * Generates the standard KMS key policy statement for the Cerberus Management Service + */ + protected Statement generateStandardCMSPolicyStatement() { + Statement cmsStatement = new Statement(Statement.Effect.Allow); + cmsStatement.withId(CERBERUS_MANAGEMENT_SERVICE_SID); + cmsStatement.withPrincipals(new Principal(AWS_PROVIDER, cmsRoleArn, false)); + cmsStatement.withActions( + KMSActions.Encrypt, + KMSActions.Decrypt, + KMSActions.ReEncryptFrom, + KMSActions.ReEncryptTo, + KMSActions.GenerateDataKey, + KMSActions.GenerateDataKeyWithoutPlaintext, + KMSActions.GenerateRandom, + KMSActions.DescribeKey, + KMSActions.ScheduleKeyDeletion, + KMSActions.CancelKeyDeletion); + cmsStatement.withResources(new Resource("*")); + + return cmsStatement; + } + public String generateStandardKmsPolicy(String iamRoleArn) { Policy kmsPolicy = new Policy(); Statement rootUserStatement = new Statement(Statement.Effect.Allow); rootUserStatement.withId("Root User Has All Actions"); rootUserStatement.withPrincipals(new Principal(AWS_PROVIDER, rootUserArn, false)); - rootUserStatement.withActions(KmsActions.AllKmsActions); + rootUserStatement.withActions(KMSActions.AllKMSActions); rootUserStatement.withResources(new Resource("*")); Statement keyAdministratorStatement = new Statement(Statement.Effect.Allow); keyAdministratorStatement.withId("Admin Role Has All Actions"); keyAdministratorStatement.withPrincipals(new Principal(AWS_PROVIDER, adminRoleArn, false)); - keyAdministratorStatement.withActions(KmsActions.AllKmsActions); + keyAdministratorStatement.withActions(KMSActions.AllKMSActions); keyAdministratorStatement.withResources(new Resource("*")); - Statement instanceUsageStatement = new Statement(Statement.Effect.Allow); - instanceUsageStatement.withId("CMS Role Key Access"); - instanceUsageStatement.withPrincipals(new Principal(AWS_PROVIDER, cmsRoleArn, false)); - instanceUsageStatement.withActions(KmsActions.EncryptAction, - KmsActions.DecryptAction, - KmsActions.AllReEncryptActions, - KmsActions.AllGenerateDataKeyActions, - KmsActions.DescribeKey); - instanceUsageStatement.withResources(new Resource("*")); + Statement instanceUsageStatement = generateStandardCMSPolicyStatement(); Statement iamRoleUsageStatement = new Statement(Statement.Effect.Allow); iamRoleUsageStatement.withId(CERBERUS_CONSUMER_SID); iamRoleUsageStatement.withPrincipals( new Principal(AWS_PROVIDER, iamRoleArn, false)); - iamRoleUsageStatement.withActions(KmsActions.DecryptAction); + iamRoleUsageStatement.withActions(KMSActions.Decrypt); iamRoleUsageStatement.withResources(new Resource("*")); kmsPolicy.withStatements(rootUserStatement, @@ -139,29 +232,4 @@ public String generateStandardKmsPolicy(String iamRoleArn) { return kmsPolicy.toJson(); } - - private enum KmsActions implements Action { - AllKmsActions("kms:*"), - - EncryptAction("kms:Encrypt"), - - DecryptAction("kms:Decrypt"), - - AllReEncryptActions("kms:ReEncrypt*"), - - AllGenerateDataKeyActions("kms:GenerateDataKey*"), - - DescribeKey("kms:DescribeKey"); - - private final String action; - - KmsActions(String action) { - this.action = action; - } - - @Override - public String getActionName() { - return action; - } - } } diff --git a/src/main/java/com/nike/cerberus/service/KmsService.java b/src/main/java/com/nike/cerberus/service/KmsService.java index 303a411d6..988d03a7c 100644 --- a/src/main/java/com/nike/cerberus/service/KmsService.java +++ b/src/main/java/com/nike/cerberus/service/KmsService.java @@ -22,7 +22,6 @@ import com.amazonaws.services.kms.model.CreateKeyRequest; import com.amazonaws.services.kms.model.CreateKeyResult; import com.amazonaws.services.kms.model.GetKeyPolicyRequest; -import com.amazonaws.services.kms.model.GetKeyPolicyResult; import com.amazonaws.services.kms.model.KeyMetadata; import com.amazonaws.services.kms.model.KeyUsageType; import com.amazonaws.services.kms.model.NotFoundException; @@ -202,19 +201,16 @@ public void validatePolicy(AwsIamRoleKmsKeyRecord kmsKeyRecord, String iamPrinci String kmsCMKRegion = kmsKeyRecord.getAwsRegion(); String awsKmsKeyArn = kmsKeyRecord.getAwsKmsKeyId(); - AWSKMSClient kmsClient = kmsClientFactory.getClient(kmsCMKRegion); try { - GetKeyPolicyResult policyResult = kmsClient.getKeyPolicy(new GetKeyPolicyRequest().withKeyId(awsKmsKeyArn).withPolicyName("default")); + String keyPolicy = getKmsKeyPolicy(awsKmsKeyArn, kmsCMKRegion); - if (!kmsPolicyService.isPolicyValid(policyResult.getPolicy(), iamPrincipalArn)) { + if (!kmsPolicyService.isPolicyValid(keyPolicy, iamPrincipalArn)) { logger.info("The KMS key: {} generated for IAM principal: {} contained an invalid policy, regenerating", awsKmsKeyArn, iamPrincipalArn); + String updatedPolicy = kmsPolicyService.generateStandardKmsPolicy(iamPrincipalArn); - kmsClient.putKeyPolicy(new PutKeyPolicyRequest() - .withKeyId(awsKmsKeyArn) - .withPolicyName("default") - .withPolicy(updatedPolicy) - ); + + updateKmsKeyPolicy(updatedPolicy, awsKmsKeyArn, kmsCMKRegion); } // update last validated timestamp @@ -231,12 +227,65 @@ public void validatePolicy(AwsIamRoleKmsKeyRecord kmsKeyRecord, String iamPrinci } } + /** + * Validate the the CMS IAM role has permissions to ScheduleKeyDeletion and CancelKeyDeletion for the give CMK + * if not, then update the policy to give the appropriate permissions. + * @param awsKmsKeyArn - The ARN of the CMK to validate + * @param kmsCMKRegion - The region that the CMK exists in + */ + protected void validatePolicyAllowsCMSToDeleteCMK(String awsKmsKeyArn, String kmsCMKRegion) { + + try { + String policyJson = getKmsKeyPolicy(awsKmsKeyArn, kmsCMKRegion); + + if (!kmsPolicyService.cmsHasKeyDeletePermissions(policyJson)) { + // only overwrite the policy statement for CMS 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); + } + } catch (AmazonServiceException ase) { + logger.error("Failed to validate that CMS can delete the given KMS key, ARN: {}, region: {}", awsKmsKeyArn, kmsCMKRegion, ase); + } catch (IllegalArgumentException iae) { + logger.error("Failed to add ScheduleKeyDeletion to key policy for CMK: {}, because the policy does" + + "not contain any allow statements for the CMS role", awsKmsKeyArn, iae); + } + + } + + /** + * Gets the KMS key policy from AWS for the given CMK + */ + protected String getKmsKeyPolicy(String awsKmsKeyArn, String kmsCMKRegion) { + + AWSKMSClient kmsClient = kmsClientFactory.getClient(kmsCMKRegion); + + GetKeyPolicyRequest request = new GetKeyPolicyRequest().withKeyId(awsKmsKeyArn).withPolicyName("default"); + + return kmsClient.getKeyPolicy(request).getPolicy(); + } + + /** + * Updates the KMS key policy in AWS for the given CMK + */ + protected void updateKmsKeyPolicy(String updatedPolicyJson, String awsKmsKeyArn, String kmsCMKRegion) { + + AWSKMSClient kmsClient = kmsClientFactory.getClient(kmsCMKRegion); + + kmsClient.putKeyPolicy(new PutKeyPolicyRequest() + .withKeyId(awsKmsKeyArn) + .withPolicyName("default") + .withPolicy(updatedPolicyJson) + ); + } + /** * Delete a CMK in AWS * @param kmsKeyId - The AWS KMS Key ID * @param region - The KMS key region */ - public void scheduleKmsKeyDeletion(String kmsKeyId, String region, Integer pendingWindowInDays) { + protected void scheduleKmsKeyDeletion(String kmsKeyId, String region, Integer pendingWindowInDays) { final AWSKMSClient kmsClient = kmsClientFactory.getClient(region); final ScheduleKeyDeletionRequest scheduleKeyDeletionRequest = new ScheduleKeyDeletionRequest() diff --git a/src/test/java/com/nike/cerberus/service/KmsPolicyServiceTest.java b/src/test/java/com/nike/cerberus/service/KmsPolicyServiceTest.java index cc3d35efa..690f4db67 100644 --- a/src/test/java/com/nike/cerberus/service/KmsPolicyServiceTest.java +++ b/src/test/java/com/nike/cerberus/service/KmsPolicyServiceTest.java @@ -1,8 +1,13 @@ package com.nike.cerberus.service; +import com.amazonaws.auth.policy.Action; +import com.amazonaws.auth.policy.Policy; +import com.amazonaws.auth.policy.Statement; +import com.amazonaws.auth.policy.actions.KMSActions; import com.fasterxml.jackson.databind.JsonNode; import com.fasterxml.jackson.databind.ObjectMapper; import org.apache.commons.io.IOUtils; +import org.apache.commons.lang3.StringUtils; import org.junit.Before; import org.junit.Test; @@ -46,29 +51,113 @@ public void test_generateStandardKmsPolicy() throws IOException { String actualPolicyJsonAsString = kmsPolicyService.generateStandardKmsPolicy(CERBERUS_CONSUMER_IAM_ROLE_ARN); assertEquals(minifiedPolicyJsonAsString, actualPolicyJsonAsString); + + expectedPolicyStream.close(); } @Test - public void test_that_generateStandardKmsPolicy_returns_true_with_a_valid_policy() throws IOException { + public void test_that_isPolicyValid_returns_true_with_a_valid_policy() 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"); assertTrue(kmsPolicyService.isPolicyValid(policyJsonAsString, CERBERUS_CONSUMER_IAM_ROLE_ARN)); + + policy.close(); } @Test - public void test_that_generateStandardKmsPolicy_returns_false_with_an_invalid_policy() throws IOException { + public void test_that_isPolicyValid_returns_false_with_an_invalid_policy() throws IOException { InputStream policy = getClass().getClassLoader() .getResourceAsStream("com/nike/cerberus/service/invalid-cerberus-iam-auth-kms-key-policy.json"); String policyJsonAsString = IOUtils.toString(policy, "UTF-8"); assertFalse(kmsPolicyService.isPolicyValid(policyJsonAsString, CERBERUS_CONSUMER_IAM_ROLE_ARN)); + + policy.close(); + } + + @Test + public void test_that_isPolicyValid_returns_false_when_cms_does_not_have_delete_permission() throws IOException { + InputStream policy = getClass().getClassLoader() + .getResourceAsStream("com/nike/cerberus/service/invalid-cerberus-kms-key-policy-cms-cannot-delete.json"); + String policyJsonAsString = IOUtils.toString(policy, "UTF-8"); + + assertFalse(kmsPolicyService.isPolicyValid(policyJsonAsString, CERBERUS_CONSUMER_IAM_ROLE_ARN)); + + policy.close(); } @Test - public void test_that_generateStandardKmsPolicy_returns_false_when_a_non_standard_policy_is_supplied() { + public void test_that_isPolicyValid_returns_false_when_a_non_standard_policy_is_supplied() { assertFalse(kmsPolicyService.isPolicyValid(null, CERBERUS_CONSUMER_IAM_ROLE_ARN)); } + @Test + public void test_that_cmsCanScheduleKeyDeletion_returns_true_when_cms_can_delete() 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"); + + assertTrue(kmsPolicyService.cmsHasKeyDeletePermissions(policyJsonAsString)); + + policy.close(); + } + + @Test + public void test_that_cmsCanScheduleKeyDeletion_returns_false_when_cms_cannot_delete() throws IOException { + InputStream policy = getClass().getClassLoader() + .getResourceAsStream("com/nike/cerberus/service/invalid-cerberus-kms-key-policy-cms-cannot-delete.json"); + String policyJsonAsString = IOUtils.toString(policy, "UTF-8"); + + assertFalse(kmsPolicyService.cmsHasKeyDeletePermissions(policyJsonAsString)); + + policy.close(); + } + + @Test + public void test_that_overwriteCMSPolicy_returns_policy_that_includes_missing_actions() throws IOException { + InputStream policy = getClass().getClassLoader() + .getResourceAsStream("com/nike/cerberus/service/invalid-cerberus-kms-key-policy-cms-cannot-delete.json"); + String policyJsonAsString = IOUtils.toString(policy, "UTF-8"); + + Action actionNotIncludedInInvalidJson1 = KMSActions.ScheduleKeyDeletion; + Action actionNotIncludedInInvalidJson2 = KMSActions.CancelKeyDeletion; + String result = kmsPolicyService.overwriteCMSPolicy(policyJsonAsString); + assertFalse(StringUtils.equals(policyJsonAsString, result)); + assertTrue(StringUtils.contains(result, actionNotIncludedInInvalidJson1.getActionName())); + assertTrue(StringUtils.contains(result, actionNotIncludedInInvalidJson2.getActionName())); + assertTrue(kmsPolicyService.cmsHasKeyDeletePermissions(result)); + + policy.close(); + } + + @Test + public void test_that_overwriteCMSPolicy_does_not_fail_with_valid_policy() 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.overwriteCMSPolicy(policyJsonAsString); + assertFalse(StringUtils.equals(policyJsonAsString, result)); + assertTrue(kmsPolicyService.cmsHasKeyDeletePermissions(result)); + + policy.close(); + } + + @Test + public void test_that_statementAllowsAction_returns_true_when_action_in_statement() { + Action action = KMSActions.CancelKeyDeletion; + Statement statement = new Statement(Statement.Effect.Allow).withActions(action); + assertTrue(kmsPolicyService.statementIncludesAction(statement, action)); + } + + @Test + public void test_that_generateStandardCMSPolicyStatement_returns_a_valid_statement() { + + Statement result = kmsPolicyService.generateStandardCMSPolicyStatement(); + assertEquals(KmsPolicyService.CERBERUS_MANAGEMENT_SERVICE_SID, result.getId()); + assertEquals(Statement.Effect.Allow, result.getEffect()); + assertTrue(kmsPolicyService.cmsHasKeyDeletePermissions(new Policy().withStatements(result).toJson())); + } } \ No newline at end of file diff --git a/src/test/resources/com/nike/cerberus/service/invalid-cerberus-iam-auth-kms-key-policy.json b/src/test/resources/com/nike/cerberus/service/invalid-cerberus-iam-auth-kms-key-policy.json index 90b00f3b8..ce58cb693 100644 --- a/src/test/resources/com/nike/cerberus/service/invalid-cerberus-iam-auth-kms-key-policy.json +++ b/src/test/resources/com/nike/cerberus/service/invalid-cerberus-iam-auth-kms-key-policy.json @@ -38,7 +38,8 @@ "kms:Decrypt", "kms:ReEncrypt*", "kms:GenerateDataKey*", - "kms:DescribeKey" + "kms:DescribeKey", + "kms:ScheduleKeyDeletion" ], "Resource":[ "*" diff --git a/src/test/resources/com/nike/cerberus/service/invalid-cerberus-kms-key-policy-cms-cannot-delete.json b/src/test/resources/com/nike/cerberus/service/invalid-cerberus-kms-key-policy-cms-cannot-delete.json new file mode 100644 index 000000000..39e7ebb68 --- /dev/null +++ b/src/test/resources/com/nike/cerberus/service/invalid-cerberus-kms-key-policy-cms-cannot-delete.json @@ -0,0 +1,61 @@ +{ + "Version":"2012-10-17", + "Statement":[ + { + "Sid":"Root User Has All Actions", + "Effect":"Allow", + "Principal":{ + "AWS":"arn:aws:iam::1111111111:root" + }, + "Action":[ + "kms:*" + ], + "Resource":[ + "*" + ] + }, + { + "Sid":"Admin Role Has All Actions", + "Effect":"Allow", + "Principal":{ + "AWS":"arn:aws:iam::1111111111:role/admin" + }, + "Action":[ + "kms:*" + ], + "Resource":[ + "*" + ] + }, + { + "Sid":"CMS Role Key Access", + "Effect":"Allow", + "Principal":{ + "AWS":"arn:aws:iam::1111111111:role/cms-iam-role" + }, + "Action":[ + "kms:Encrypt", + "kms:Decrypt", + "kms:ReEncrypt*", + "kms:GenerateDataKey*", + "kms:DescribeKey" + ], + "Resource":[ + "*" + ] + }, + { + "Sid":"Target IAM Role Has Decrypt Action", + "Effect":"Allow", + "Principal":{ + "AWS":"arn:aws:iam::1234567890:role/cerberus-consumer" + }, + "Action":[ + "kms:Decrypt" + ], + "Resource":[ + "*" + ] + } + ] +} \ No newline at end of file diff --git a/src/test/resources/com/nike/cerberus/service/valid-cerberus-iam-auth-kms-key-policy.json b/src/test/resources/com/nike/cerberus/service/valid-cerberus-iam-auth-kms-key-policy.json index 39e7ebb68..1e1454b00 100644 --- a/src/test/resources/com/nike/cerberus/service/valid-cerberus-iam-auth-kms-key-policy.json +++ b/src/test/resources/com/nike/cerberus/service/valid-cerberus-iam-auth-kms-key-policy.json @@ -36,9 +36,14 @@ "Action":[ "kms:Encrypt", "kms:Decrypt", - "kms:ReEncrypt*", - "kms:GenerateDataKey*", - "kms:DescribeKey" + "kms:ReEncryptFrom", + "kms:ReEncryptTo", + "kms:GenerateDataKey", + "kms:GenerateDataKeyWithoutPlaintext", + "kms:GenerateRandom", + "kms:DescribeKey", + "kms:ScheduleKeyDeletion", + "kms:CancelKeyDeletion" ], "Resource":[ "*" From cd7ee6480b4d2acaf32dbb86ca7abeb0dfc5dcaf Mon Sep 17 00:00:00 2001 From: Shaun Ford Date: Thu, 13 Jul 2017 09:21:12 -0700 Subject: [PATCH 2/4] Immediately return from cleanup endpoint * Update the cleanup endpoint to immediately return 204 (No Content) status code, and run actual clean up process in the background --- .../CleanUpInactiveOrOrphanedRecords.java | 28 ++++++------------- .../nike/cerberus/service/CleanUpService.java | 15 ++++++++-- 2 files changed, 22 insertions(+), 21 deletions(-) diff --git a/src/main/java/com/nike/cerberus/endpoints/admin/CleanUpInactiveOrOrphanedRecords.java b/src/main/java/com/nike/cerberus/endpoints/admin/CleanUpInactiveOrOrphanedRecords.java index e08b72aac..94b742c44 100644 --- a/src/main/java/com/nike/cerberus/endpoints/admin/CleanUpInactiveOrOrphanedRecords.java +++ b/src/main/java/com/nike/cerberus/endpoints/admin/CleanUpInactiveOrOrphanedRecords.java @@ -23,7 +23,6 @@ import com.nike.cerberus.service.CleanUpService; import com.nike.riposte.server.http.RequestInfo; import com.nike.riposte.server.http.ResponseInfo; -import com.nike.riposte.server.http.impl.FullResponseInfo; import com.nike.riposte.util.AsyncNettyHelper; import com.nike.riposte.util.Matcher; import io.netty.channel.ChannelHandlerContext; @@ -50,8 +49,6 @@ public class CleanUpInactiveOrOrphanedRecords extends AdminStandardEndpoint> doExecute(final RequestInfo cleanUp(request, securityContext), ctx), - longRunningTaskExecutor - ); - } - - private FullResponseInfo cleanUp(final RequestInfo request, - final SecurityContext securityContext) { final VaultAuthPrincipal vaultAuthPrincipal = (VaultAuthPrincipal) securityContext.getUserPrincipal(); final String principal = vaultAuthPrincipal.getName(); log.info("Clean Up Event: the principal {} is attempting to clean up kms keys", principal); - Integer expirationPeriodInDays = request.getContent().getKmsExpirationPeriodInDays(); - int kmsKeysInactiveAfterNDays = (expirationPeriodInDays == null) ? DEFAULT_KMS_KEY_INACTIVE_AFTER_N_DAYS : expirationPeriodInDays; - - cleanUpService.cleanUpInactiveAndOrphanedKmsKeys(kmsKeysInactiveAfterNDays); - cleanUpService.cleanUpOrphanedIamRoles(); + longRunningTaskExecutor.execute(AsyncNettyHelper.runnableWithTracingAndMdc( + () -> cleanUpService.cleanUp(request.getContent()), + ctx + )); - return ResponseInfo.newBuilder() - .withHttpStatusCode(HttpResponseStatus.NO_CONTENT.code()) - .build(); + return CompletableFuture.completedFuture( + ResponseInfo.newBuilder() + .withHttpStatusCode(HttpResponseStatus.NO_CONTENT.code()) + .build() + ); } @Override diff --git a/src/main/java/com/nike/cerberus/service/CleanUpService.java b/src/main/java/com/nike/cerberus/service/CleanUpService.java index aa2f5e789..af97b83f7 100644 --- a/src/main/java/com/nike/cerberus/service/CleanUpService.java +++ b/src/main/java/com/nike/cerberus/service/CleanUpService.java @@ -20,6 +20,7 @@ import com.google.inject.Inject; import com.google.inject.Singleton; import com.nike.cerberus.dao.AwsIamRoleDao; +import com.nike.cerberus.domain.CleanUpRequest; import com.nike.cerberus.record.AwsIamRoleKmsKeyRecord; import com.nike.cerberus.record.AwsIamRoleRecord; import com.nike.cerberus.util.DateTimeSupplier; @@ -42,6 +43,8 @@ public class CleanUpService { private static final int DEFAULT_SLEEP_BETWEEN_KMS_CALLS = 10; // in seconds + private static final int DEFAULT_KMS_KEY_INACTIVE_AFTER_N_DAYS = 30; + private final KmsService kmsService; private final AwsIamRoleDao awsIamRoleDao; @@ -57,11 +60,19 @@ public CleanUpService(KmsService kmsService, this.dateTimeSupplier = dateTimeSupplier; } + public void cleanUp(final CleanUpRequest cleanUpRequest) { + Integer expirationPeriodInDays = cleanUpRequest.getKmsExpirationPeriodInDays(); + int kmsKeysInactiveAfterNDays = (expirationPeriodInDays == null) ? DEFAULT_KMS_KEY_INACTIVE_AFTER_N_DAYS : expirationPeriodInDays; + + cleanUpInactiveAndOrphanedKmsKeys(kmsKeysInactiveAfterNDays); + cleanUpOrphanedIamRoles(); + } + /** * Delete all AWS KMS keys and DB records for KMS keys that have not been used recently * or are no longer associated with an SDB. */ - public void cleanUpInactiveAndOrphanedKmsKeys(final int kmsKeysInactiveAfterNDays) { + protected void cleanUpInactiveAndOrphanedKmsKeys(final int kmsKeysInactiveAfterNDays) { cleanUpInactiveAndOrphanedKmsKeys(kmsKeysInactiveAfterNDays, DEFAULT_SLEEP_BETWEEN_KMS_CALLS); } @@ -109,7 +120,7 @@ protected void cleanUpInactiveAndOrphanedKmsKeys(final int kmsKeysInactiveAfterN /** * Delete all IAM role records that are no longer associated with an SDB. */ - public void cleanUpOrphanedIamRoles() { + protected void cleanUpOrphanedIamRoles() { // get orphaned iam role ids final List orphanedIamRoleIds = awsIamRoleDao.getOrphanedIamRoles(); From bff872b6ba2521447d8204717fee1eefc00dd654 Mon Sep 17 00:00:00 2001 From: Shaun Ford Date: Thu, 13 Jul 2017 11:01:14 -0700 Subject: [PATCH 3/4] 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 From 25d01685ec905e4259284351084272a7b239136f Mon Sep 17 00:00:00 2001 From: Shaun Ford Date: Thu, 13 Jul 2017 16:50:18 -0700 Subject: [PATCH 4/4] @Transational on DB update methods + validate keys * Add transactional annotations where missing on methods that update records in the database * Validate that KMS keys are usable during authentication. If not, then delete the key record --- .../service/AuthenticationService.java | 1 + .../nike/cerberus/service/CleanUpService.java | 4 +- .../com/nike/cerberus/service/KmsService.java | 75 +++++++-- .../cerberus/service/CleanUpServiceTest.java | 2 +- .../nike/cerberus/service/KmsServiceTest.java | 145 +++++++++++++++++- 5 files changed, 214 insertions(+), 13 deletions(-) diff --git a/src/main/java/com/nike/cerberus/service/AuthenticationService.java b/src/main/java/com/nike/cerberus/service/AuthenticationService.java index d49a05b63..0167034e5 100644 --- a/src/main/java/com/nike/cerberus/service/AuthenticationService.java +++ b/src/main/java/com/nike/cerberus/service/AuthenticationService.java @@ -498,6 +498,7 @@ protected String getKeyId(IamPrincipalCredentials credentials) { kmsKeyRecord = kmsKey.get(); kmsKeyId = kmsKeyRecord.getAwsKmsKeyId(); kmsService.validatePolicy(kmsKeyRecord, iamPrincipalArn); + kmsService.validateKmsKeyIsUsable(kmsKeyRecord, iamPrincipalArn); } return kmsKeyId; diff --git a/src/main/java/com/nike/cerberus/service/CleanUpService.java b/src/main/java/com/nike/cerberus/service/CleanUpService.java index af97b83f7..20d3f562d 100644 --- a/src/main/java/com/nike/cerberus/service/CleanUpService.java +++ b/src/main/java/com/nike/cerberus/service/CleanUpService.java @@ -24,6 +24,7 @@ import com.nike.cerberus.record.AwsIamRoleKmsKeyRecord; import com.nike.cerberus.record.AwsIamRoleRecord; import com.nike.cerberus.util.DateTimeSupplier; +import org.mybatis.guice.transactional.Transactional; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -102,7 +103,7 @@ protected void cleanUpInactiveAndOrphanedKmsKeys(final int kmsKeysInactiveAfterN kmsKeyArn, kmsKeyRegion, kmsKeyRecord.getLastValidatedTs()); kmsService.validatePolicyAllowsCMSToDeleteCMK(kmsKeyArn, kmsKeyRegion); kmsService.scheduleKmsKeyDeletion(kmsKeyArn, kmsKeyRegion, SOONEST_A_KMS_KEY_CAN_BE_DELETED); - awsIamRoleDao.deleteKmsKeyById(kmsKeyRecord.getId()); + kmsService.deleteKmsKeyById(kmsKeyRecord.getId()); TimeUnit.SECONDS.sleep(sleepInSeconds); } catch (InterruptedException ie) { logger.error("Timeout between KMS key deletion was interrupted", ie); @@ -120,6 +121,7 @@ protected void cleanUpInactiveAndOrphanedKmsKeys(final int kmsKeysInactiveAfterN /** * Delete all IAM role records that are no longer associated with an SDB. */ + @Transactional protected void cleanUpOrphanedIamRoles() { // get orphaned iam role ids diff --git a/src/main/java/com/nike/cerberus/service/KmsService.java b/src/main/java/com/nike/cerberus/service/KmsService.java index b97ec2cfc..dd28d8725 100644 --- a/src/main/java/com/nike/cerberus/service/KmsService.java +++ b/src/main/java/com/nike/cerberus/service/KmsService.java @@ -21,8 +21,10 @@ import com.amazonaws.services.kms.model.CreateAliasRequest; import com.amazonaws.services.kms.model.CreateKeyRequest; import com.amazonaws.services.kms.model.CreateKeyResult; +import com.amazonaws.services.kms.model.DescribeKeyRequest; import com.amazonaws.services.kms.model.GetKeyPolicyRequest; import com.amazonaws.services.kms.model.KeyMetadata; +import com.amazonaws.services.kms.model.KeyState; import com.amazonaws.services.kms.model.KeyUsageType; import com.amazonaws.services.kms.model.NotFoundException; import com.amazonaws.services.kms.model.PutKeyPolicyRequest; @@ -35,6 +37,7 @@ import com.nike.cerberus.record.AwsIamRoleKmsKeyRecord; import com.nike.cerberus.util.DateTimeSupplier; import com.nike.cerberus.util.UuidSupplier; +import org.apache.commons.lang3.StringUtils; import org.mybatis.guice.transactional.Transactional; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -173,6 +176,11 @@ public void updateKmsKey(final String awsIamRoleId, awsIamRoleDao.updateIamRoleKmsKey(updatedKmsKeyRecord); } + @Transactional + public void deleteKmsKeyById(final String kmsKeyId) { + awsIamRoleDao.deleteKmsKeyById(kmsKeyId); + } + protected String getAliasName(String awsIamRoleKmsKeyId) { return String.format(KMS_ALIAS_FORMAT, awsIamRoleKmsKeyId); } @@ -220,7 +228,7 @@ public void validatePolicy(AwsIamRoleKmsKeyRecord kmsKeyRecord, String iamPrinci logger.warn("Failed to validate KMS policy because the KMS key did not exist, but the key record did." + "Deleting the key record to prevent this from failing again: keyId: {} for IAM principal: {} in region: {}", awsKmsKeyArn, iamPrincipalArn, kmsCMKRegion, nfe); - awsIamRoleDao.deleteKmsKeyById(kmsKeyRecord.getId()); + deleteKmsKeyById(kmsKeyRecord.getId()); } catch (AmazonServiceException e) { logger.warn(String.format("Failed to validate KMS policy for keyId: %s for IAM principal: %s in region: %s. API limit" + " may have been reached for validate call.", awsKmsKeyArn, iamPrincipalArn, kmsCMKRegion), e); @@ -230,13 +238,13 @@ public void validatePolicy(AwsIamRoleKmsKeyRecord kmsKeyRecord, String iamPrinci /** * Validate the the CMS IAM role has permissions to ScheduleKeyDeletion and CancelKeyDeletion for the give CMK * if not, then update the policy to give the appropriate permissions. - * @param awsKmsKeyArn - The ARN of the CMK to validate + * @param awsKmsKeyId - The ARN of the CMK to validate * @param kmsCMKRegion - The region that the CMK exists in */ - protected void validatePolicyAllowsCMSToDeleteCMK(String awsKmsKeyArn, String kmsCMKRegion) { + protected void validatePolicyAllowsCMSToDeleteCMK(String awsKmsKeyId, String kmsCMKRegion) { try { - String policyJson = getKmsKeyPolicy(awsKmsKeyArn, kmsCMKRegion); + String policyJson = getKmsKeyPolicy(awsKmsKeyId, kmsCMKRegion); if (!kmsPolicyService.cmsHasKeyDeletePermissions(policyJson)) { // Overwrite the policy statement for CMS only, instead of regenerating the entire policy because regenerating @@ -247,25 +255,58 @@ protected void validatePolicyAllowsCMSToDeleteCMK(String awsKmsKeyArn, String km // 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); + updateKmsKeyPolicy(updatedPolicyWithNoConsumer, awsKmsKeyId, kmsCMKRegion); } } catch (AmazonServiceException ase) { - logger.error("Failed to validate that CMS can delete the given KMS key, ARN: {}, region: {}", awsKmsKeyArn, kmsCMKRegion, ase); + logger.error("Failed to validate that CMS can delete the given KMS key, ARN: {}, region: {}", awsKmsKeyId, kmsCMKRegion, ase); } catch (IllegalArgumentException iae) { logger.error("Failed to add ScheduleKeyDeletion to key policy for CMK: {}, because the policy does" + - "not contain any allow statements for the CMS role", awsKmsKeyArn, iae); + "not contain any allow statements for the CMS role", awsKmsKeyId, iae); + } + + } + + /** + * Validate that the KMS key is usable (i.e. the key is not disabled or scheduled for deletion). + * + * If the key is not usable, then delete the DB record of the key, so that the caller will be given a new KMS key + * the next time they authenticate. + * + * @param kmsKeyRecord - Record of the KMS key to validate + * @param iamPrincipalArn - ARN of the authenticating IAM principal + */ + protected void validateKmsKeyIsUsable(AwsIamRoleKmsKeyRecord kmsKeyRecord, String iamPrincipalArn) { + + String kmsCMKRegion = kmsKeyRecord.getAwsRegion(); + String awsKmsKeyArn = kmsKeyRecord.getAwsKmsKeyId(); + + if (kmsKeyIsDisabledOrScheduledForDeletion(awsKmsKeyArn, kmsCMKRegion)) { + 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()); } + } + /** + * Return true if the KMS key is disabled or scheduled for deletion, false if not. + */ + protected boolean kmsKeyIsDisabledOrScheduledForDeletion(String awsKmsKeyId, String kmsCMKRegion) { + + String keyState = getKmsKeyState(awsKmsKeyId, kmsCMKRegion); + + return StringUtils.equals(keyState, KeyState.PendingDeletion.toString()) || + StringUtils.equals(keyState, KeyState.Disabled.toString()); } /** * Gets the KMS key policy from AWS for the given CMK */ - protected String getKmsKeyPolicy(String awsKmsKeyArn, String kmsCMKRegion) { + protected String getKmsKeyPolicy(String kmsKeyId, String kmsCMKRegion) { AWSKMSClient kmsClient = kmsClientFactory.getClient(kmsCMKRegion); - GetKeyPolicyRequest request = new GetKeyPolicyRequest().withKeyId(awsKmsKeyArn).withPolicyName("default"); + GetKeyPolicyRequest request = new GetKeyPolicyRequest().withKeyId(kmsKeyId).withPolicyName("default"); return kmsClient.getKeyPolicy(request).getPolicy(); } @@ -284,6 +325,22 @@ protected void updateKmsKeyPolicy(String updatedPolicyJson, String awsKmsKeyArn, ); } + /** + * Get the state of the KMS key + * @param kmsKeyId - The AWS KMS Key ID + * @param region - The KMS key region + * @return - KMS key state + */ + protected String getKmsKeyState(String kmsKeyId, String region) { + + AWSKMSClient kmsClient = kmsClientFactory.getClient(region); + DescribeKeyRequest request = new DescribeKeyRequest().withKeyId(kmsKeyId); + + return kmsClient.describeKey(request) + .getKeyMetadata() + .getKeyState(); + } + /** * Delete a CMK in AWS * @param kmsKeyId - The AWS KMS Key ID diff --git a/src/test/java/com/nike/cerberus/service/CleanUpServiceTest.java b/src/test/java/com/nike/cerberus/service/CleanUpServiceTest.java index b2f71727b..abc09c322 100644 --- a/src/test/java/com/nike/cerberus/service/CleanUpServiceTest.java +++ b/src/test/java/com/nike/cerberus/service/CleanUpServiceTest.java @@ -66,7 +66,7 @@ public void test_that_cleanUpInactiveAndOrphanedKmsKeys_succeeds() { cleanUpService.cleanUpInactiveAndOrphanedKmsKeys(inactivePeriod, 0); verify(awsIamRoleDao).getInactiveOrOrphanedKmsKeys(inactiveCutoffDate); - verify(awsIamRoleDao).deleteKmsKeyById(keyRecordId); + verify(kmsService).deleteKmsKeyById(keyRecordId); verify(kmsService).scheduleKmsKeyDeletion(awsKeyId, keyRegion, SOONEST_A_KMS_KEY_CAN_BE_DELETED); } diff --git a/src/test/java/com/nike/cerberus/service/KmsServiceTest.java b/src/test/java/com/nike/cerberus/service/KmsServiceTest.java index 575ca9242..d46e2f4d1 100644 --- a/src/test/java/com/nike/cerberus/service/KmsServiceTest.java +++ b/src/test/java/com/nike/cerberus/service/KmsServiceTest.java @@ -2,7 +2,15 @@ import com.amazonaws.AmazonServiceException; import com.amazonaws.services.kms.AWSKMSClient; -import com.amazonaws.services.kms.model.*; +import com.amazonaws.services.kms.model.CreateAliasRequest; +import com.amazonaws.services.kms.model.CreateKeyRequest; +import com.amazonaws.services.kms.model.CreateKeyResult; +import com.amazonaws.services.kms.model.DescribeKeyResult; +import com.amazonaws.services.kms.model.GetKeyPolicyRequest; +import com.amazonaws.services.kms.model.GetKeyPolicyResult; +import com.amazonaws.services.kms.model.KeyMetadata; +import com.amazonaws.services.kms.model.KeyState; +import com.amazonaws.services.kms.model.KeyUsageType; import com.nike.backstopper.exception.ApiException; import com.nike.cerberus.aws.KmsClientFactory; import com.nike.cerberus.dao.AwsIamRoleDao; @@ -17,7 +25,15 @@ import java.util.Optional; import static org.junit.Assert.assertEquals; -import static org.mockito.Mockito.*; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; +import static org.mockito.Mockito.anyObject; +import static org.mockito.Mockito.anyString; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; public class KmsServiceTest { @@ -221,4 +237,129 @@ public void test_updateKmsKey_fails_when_record_not_found() { kmsService.updateKmsKey(iamRoleId, awsRegion, user, dateTime, dateTime); } + + @Test + public void test_getKmsKeyState_happy() { + String awsRegion = "aws region"; + + String kmsKeyId = "kms key id"; + String state = "state"; + AWSKMSClient kmsClient = mock(AWSKMSClient.class); + when(kmsClientFactory.getClient(awsRegion)).thenReturn(kmsClient); + when(kmsClient.describeKey(anyObject())).thenReturn( + new DescribeKeyResult() + .withKeyMetadata( + new KeyMetadata() + .withKeyState(state))); + + String result = kmsService.getKmsKeyState(kmsKeyId, awsRegion); + + assertEquals(state, result); + } + + @Test + public void test_validateKmsKeyIsUsable_returns_true_when_state_is_pending_deletion() { + String keyId = "key id"; + String awsRegion = "aws region"; + + AWSKMSClient kmsClient = mock(AWSKMSClient.class); + when(kmsClientFactory.getClient(awsRegion)).thenReturn(kmsClient); + when(kmsClient.describeKey(anyObject())).thenReturn( + new DescribeKeyResult() + .withKeyMetadata( + new KeyMetadata() + .withKeyState(KeyState.PendingDeletion))); + + boolean result = kmsService.kmsKeyIsDisabledOrScheduledForDeletion(keyId, awsRegion); + + assertTrue(result); + } + + @Test + public void test_validateKmsKeyIsUsable_return_true_when_state_is_disabled() { + String keyId = "key id"; + String awsRegion = "aws region"; + + AWSKMSClient kmsClient = mock(AWSKMSClient.class); + when(kmsClientFactory.getClient(awsRegion)).thenReturn(kmsClient); + when(kmsClient.describeKey(anyObject())).thenReturn( + new DescribeKeyResult() + .withKeyMetadata( + new KeyMetadata() + .withKeyState(KeyState.Disabled))); + + boolean result = kmsService.kmsKeyIsDisabledOrScheduledForDeletion(keyId, awsRegion); + + assertTrue(result); + } + + @Test + public void test_validateKmsKeyIsUsable_returns_false_when_state_is_not_deletion_or_disabled() { + String keyId = "key id"; + String awsRegion = "aws region"; + + AWSKMSClient kmsClient = mock(AWSKMSClient.class); + when(kmsClientFactory.getClient(awsRegion)).thenReturn(kmsClient); + when(kmsClient.describeKey(anyObject())).thenReturn( + new DescribeKeyResult() + .withKeyMetadata( + new KeyMetadata() + .withKeyState(KeyState.Enabled))); + + boolean result = kmsService.kmsKeyIsDisabledOrScheduledForDeletion(keyId, awsRegion); + + assertFalse(result); + } + + @Test + public void test_validateKmsKeyIsUsable_deletes_kms_key_when_not_usable() { + + String id = "id"; + String awsKmsKeyArn = "aws kms key arn"; + String iamPrincipalArn = "arn"; + String awsRegion = "aws region"; + + AwsIamRoleKmsKeyRecord kmsKey = mock(AwsIamRoleKmsKeyRecord.class); + when(kmsKey.getId()).thenReturn(id); + when(kmsKey.getAwsKmsKeyId()).thenReturn(awsKmsKeyArn); + when(kmsKey.getAwsRegion()).thenReturn(awsRegion); + + AWSKMSClient kmsClient = mock(AWSKMSClient.class); + when(kmsClientFactory.getClient(awsRegion)).thenReturn(kmsClient); + when(kmsClient.describeKey(anyObject())).thenReturn( + new DescribeKeyResult() + .withKeyMetadata( + new KeyMetadata() + .withKeyState(KeyState.PendingDeletion))); + + kmsService.validateKmsKeyIsUsable(kmsKey, iamPrincipalArn); + + verify(awsIamRoleDao, times(1)).deleteKmsKeyById(id); + } + + @Test + public void test_validateKmsKeyIsUsable_does_not_delete_kms_key_when_usable() { + + String id = "id"; + String awsKmsKeyArn = "aws kms key arn"; + String iamPrincipalArn = "arn"; + String awsRegion = "aws region"; + + AwsIamRoleKmsKeyRecord kmsKey = mock(AwsIamRoleKmsKeyRecord.class); + when(kmsKey.getId()).thenReturn(id); + when(kmsKey.getAwsKmsKeyId()).thenReturn(awsKmsKeyArn); + when(kmsKey.getAwsRegion()).thenReturn(awsRegion); + + AWSKMSClient kmsClient = mock(AWSKMSClient.class); + when(kmsClientFactory.getClient(awsRegion)).thenReturn(kmsClient); + when(kmsClient.describeKey(anyObject())).thenReturn( + new DescribeKeyResult() + .withKeyMetadata( + new KeyMetadata() + .withKeyState(KeyState.Enabled))); + + kmsService.validateKmsKeyIsUsable(kmsKey, iamPrincipalArn); + + verify(awsIamRoleDao, never()).deleteKmsKeyById(id); + } } \ No newline at end of file