From 25d01685ec905e4259284351084272a7b239136f Mon Sep 17 00:00:00 2001 From: Shaun Ford Date: Thu, 13 Jul 2017 16:50:18 -0700 Subject: [PATCH] @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