From b04a23a69aed055443a736e476f6f0c41298724f Mon Sep 17 00:00:00 2001 From: Todd Lisonbee Date: Fri, 21 Jul 2017 08:54:10 -0700 Subject: [PATCH] Moving key validation to avoid being rate limited by AWS --- .../nike/cerberus/error/DefaultApiError.java | 5 +++++ .../service/AuthenticationService.java | 3 +-- .../com/nike/cerberus/service/KmsService.java | 11 +++++++++- .../service/AuthenticationServiceTest.java | 2 +- .../nike/cerberus/service/KmsServiceTest.java | 20 +++++++++++-------- 5 files changed, 29 insertions(+), 12 deletions(-) diff --git a/src/main/java/com/nike/cerberus/error/DefaultApiError.java b/src/main/java/com/nike/cerberus/error/DefaultApiError.java index 1bcaf3e6b..6b91e9741 100644 --- a/src/main/java/com/nike/cerberus/error/DefaultApiError.java +++ b/src/main/java/com/nike/cerberus/error/DefaultApiError.java @@ -232,6 +232,11 @@ public enum DefaultApiError implements ApiError { */ IAM_PRINCIPALS_CANNOT_USE_USER_ONLY_RESOURCE(99231, "The requested resource is for User Principals only.", HttpServletResponse.SC_FORBIDDEN), + /** + * KMS key is scheduled for deletion or disabled + */ + KMS_KEY_IS_SCHEDULED_FOR_DELETION_OR_DISABLED(99232, "KMS key is scheduled for deletion or disabled", HttpServletResponse.SC_INTERNAL_SERVER_ERROR), + /** * Generic not found error. */ diff --git a/src/main/java/com/nike/cerberus/service/AuthenticationService.java b/src/main/java/com/nike/cerberus/service/AuthenticationService.java index 0167034e5..b5a16d552 100644 --- a/src/main/java/com/nike/cerberus/service/AuthenticationService.java +++ b/src/main/java/com/nike/cerberus/service/AuthenticationService.java @@ -497,8 +497,7 @@ protected String getKeyId(IamPrincipalCredentials credentials) { } else { kmsKeyRecord = kmsKey.get(); kmsKeyId = kmsKeyRecord.getAwsKmsKeyId(); - kmsService.validatePolicy(kmsKeyRecord, iamPrincipalArn); - kmsService.validateKmsKeyIsUsable(kmsKeyRecord, iamPrincipalArn); + kmsService.validateKeyAndPolicy(kmsKeyRecord, iamPrincipalArn); } return kmsKeyId; diff --git a/src/main/java/com/nike/cerberus/service/KmsService.java b/src/main/java/com/nike/cerberus/service/KmsService.java index dd28d8725..7a003200d 100644 --- a/src/main/java/com/nike/cerberus/service/KmsService.java +++ b/src/main/java/com/nike/cerberus/service/KmsService.java @@ -186,6 +186,8 @@ protected String getAliasName(String awsIamRoleKmsKeyId) { } /** + * Perform validation and fix some issues. + * * When a KMS key policy statement is created and an AWS ARN is specified as a principal, * AWS behind the scene binds that ARNs ID to the statement and not the ARN. * @@ -201,9 +203,10 @@ protected String getAliasName(String awsIamRoleKmsKeyId) { * @param kmsKeyRecord - The CMK record to validate policy on * @param iamPrincipalArn - The principal ARN that should have decrypt permission */ - public void validatePolicy(AwsIamRoleKmsKeyRecord kmsKeyRecord, String iamPrincipalArn) { + public void validateKeyAndPolicy(AwsIamRoleKmsKeyRecord kmsKeyRecord, String iamPrincipalArn) { if (! kmsPolicyNeedsValidation(kmsKeyRecord)) { + // Avoiding extra calls to AWS so that we don't get rate limited. return; } @@ -221,6 +224,8 @@ public void validatePolicy(AwsIamRoleKmsKeyRecord kmsKeyRecord, String iamPrinci updateKmsKeyPolicy(updatedPolicy, awsKmsKeyArn, kmsCMKRegion); } + validateKmsKeyIsUsable(kmsKeyRecord, iamPrincipalArn); + // update last validated timestamp OffsetDateTime now = dateTimeSupplier.get(); updateKmsKey(kmsKeyRecord.getAwsIamRoleId(), kmsCMKRegion, SYSTEM_USER, now, now); @@ -281,10 +286,14 @@ protected void validateKmsKeyIsUsable(AwsIamRoleKmsKeyRecord kmsKeyRecord, Strin String awsKmsKeyArn = kmsKeyRecord.getAwsKmsKeyId(); if (kmsKeyIsDisabledOrScheduledForDeletion(awsKmsKeyArn, kmsCMKRegion)) { + // This shouldn't happen unless there is a bug in CMS or if someone modified the key outside of Cerberus 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()); + throw ApiException.newBuilder() + .withApiErrors(DefaultApiError.KMS_KEY_IS_SCHEDULED_FOR_DELETION_OR_DISABLED) + .build(); } } diff --git a/src/test/java/com/nike/cerberus/service/AuthenticationServiceTest.java b/src/test/java/com/nike/cerberus/service/AuthenticationServiceTest.java index 0d4b31442..cc3c89171 100644 --- a/src/test/java/com/nike/cerberus/service/AuthenticationServiceTest.java +++ b/src/test/java/com/nike/cerberus/service/AuthenticationServiceTest.java @@ -169,7 +169,7 @@ public void test_that_getKeyId_only_validates_kms_policy_one_time_within_interva // verify validate is called once interval has passed assertEquals(cmkId, result); - verify(kmsService, times(1)).validatePolicy(awsIamRoleKmsKeyRecord, principalArn); + verify(kmsService, times(1)).validateKeyAndPolicy(awsIamRoleKmsKeyRecord, principalArn); } @Test diff --git a/src/test/java/com/nike/cerberus/service/KmsServiceTest.java b/src/test/java/com/nike/cerberus/service/KmsServiceTest.java index d46e2f4d1..b3ea4b262 100644 --- a/src/test/java/com/nike/cerberus/service/KmsServiceTest.java +++ b/src/test/java/com/nike/cerberus/service/KmsServiceTest.java @@ -125,6 +125,12 @@ public void test_validatePolicy_validates_policy_when_validate_interval_has_pass OffsetDateTime now = OffsetDateTime.now(); AWSKMSClient client = mock(AWSKMSClient.class); + when(client.describeKey(anyObject())).thenReturn( + new DescribeKeyResult() + .withKeyMetadata( + new KeyMetadata() + .withKeyState(KeyState.Enabled))); + when(kmsClientFactory.getClient(kmsCMKRegion)).thenReturn(client); GetKeyPolicyResult result = mock(GetKeyPolicyResult.class); @@ -141,7 +147,7 @@ public void test_validatePolicy_validates_policy_when_validate_interval_has_pass when(awsIamRoleDao.getKmsKey(awsIamRoleRecordId, kmsCMKRegion)).thenReturn(Optional.of(kmsKey)); when(dateTimeSupplier.get()).thenReturn(now); - kmsService.validatePolicy(kmsKey, kmsKeyArn); + kmsService.validateKeyAndPolicy(kmsKey, kmsKeyArn); verify(client, times(1)).getKeyPolicy(new GetKeyPolicyRequest().withKeyId(kmsKeyArn) .withPolicyName("default")); @@ -149,7 +155,7 @@ public void test_validatePolicy_validates_policy_when_validate_interval_has_pass } @Test - public void test_validatePolicy_validates_policy_when_validate_interval_has_not_passed() { + public void test_validateKeyAndPolicy_validates_policy_when_validate_interval_has_not_passed() { String awsKmsKeyArn = "aws kms key arn"; String iamPrincipalArn = "arn"; String awsIamRoleRecordId = "aws iam role record id"; @@ -163,14 +169,14 @@ public void test_validatePolicy_validates_policy_when_validate_interval_has_not_ when(kmsKey.getLastValidatedTs()).thenReturn(now); when(dateTimeSupplier.get()).thenReturn(now); - kmsService.validatePolicy(kmsKey, iamPrincipalArn); + kmsService.validateKeyAndPolicy(kmsKey, iamPrincipalArn); verify(kmsClientFactory, never()).getClient(anyString()); verify(kmsPolicyService, never()).isPolicyValid(anyString(), anyString()); } @Test - public void test_validatePolicy_does_not_throw_error_when_cannot_validate() { + public void test_validateKeyAndPolicy_does_not_throw_error_when_cannot_validate() { String keyId = "key-id"; String iamPrincipalArn = "arn"; String kmsCMKRegion = "kmsCMKRegion"; @@ -193,7 +199,7 @@ public void test_validatePolicy_does_not_throw_error_when_cannot_validate() { when(result.getPolicy()).thenReturn(policy); when(client.getKeyPolicy(new GetKeyPolicyRequest().withKeyId(keyId).withPolicyName("default"))).thenThrow(AmazonServiceException.class); - kmsService.validatePolicy(kmsKey, iamPrincipalArn); + kmsService.validateKeyAndPolicy(kmsKey, iamPrincipalArn); verify(kmsPolicyService, never()).isPolicyValid(policy, iamPrincipalArn); verify(client, never()).putKeyPolicy(anyObject()); @@ -311,7 +317,7 @@ public void test_validateKmsKeyIsUsable_returns_false_when_state_is_not_deletion assertFalse(result); } - @Test + @Test(expected = ApiException.class) public void test_validateKmsKeyIsUsable_deletes_kms_key_when_not_usable() { String id = "id"; @@ -333,8 +339,6 @@ public void test_validateKmsKeyIsUsable_deletes_kms_key_when_not_usable() { .withKeyState(KeyState.PendingDeletion))); kmsService.validateKmsKeyIsUsable(kmsKey, iamPrincipalArn); - - verify(awsIamRoleDao, times(1)).deleteKmsKeyById(id); } @Test