From 8ba9fb4a29b09aa59aa2efde983f8b0df6faa651 Mon Sep 17 00:00:00 2001 From: Justin Field Date: Mon, 2 Oct 2017 14:14:42 -0700 Subject: [PATCH] make policy checker check for arn rather than specific id, as to avoid unnessary calls to KMS (#70) --- .../cerberus/service/KmsPolicyService.java | 20 ++++++++++++------- .../com/nike/cerberus/service/KmsService.java | 2 +- .../service/KmsPolicyServiceTest.java | 11 +++++----- .../nike/cerberus/service/KmsServiceTest.java | 8 ++++---- 4 files changed, 24 insertions(+), 17 deletions(-) diff --git a/src/main/java/com/nike/cerberus/service/KmsPolicyService.java b/src/main/java/com/nike/cerberus/service/KmsPolicyService.java index 44cfcf0d8..6e95a7536 100644 --- a/src/main/java/com/nike/cerberus/service/KmsPolicyService.java +++ b/src/main/java/com/nike/cerberus/service/KmsPolicyService.java @@ -25,6 +25,7 @@ import com.amazonaws.auth.policy.actions.KMSActions; import com.amazonaws.auth.policy.internal.JsonPolicyReader; import com.amazonaws.services.kms.model.PutKeyPolicyRequest; +import com.nike.cerberus.util.AwsIamRoleArnParser; import org.apache.commons.lang3.StringUtils; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -64,13 +65,18 @@ public class KmsPolicyService { private final JsonPolicyReader policyReader; + private final AwsIamRoleArnParser awsIamRoleArnParser; + @Inject public KmsPolicyService(@Named(ROOT_USER_ARN_PROPERTY) String rootUserArn, @Named(ADMIN_ROLE_ARN_PROPERTY) String adminRoleArn, - @Named(CMS_ROLE_ARN_PROPERTY) String cmsRoleArn) { + @Named(CMS_ROLE_ARN_PROPERTY) String cmsRoleArn, + AwsIamRoleArnParser awsIamRoleArnParser) { + this.rootUserArn = rootUserArn; this.adminRoleArn = adminRoleArn; this.cmsRoleArn = cmsRoleArn; + this.awsIamRoleArnParser = awsIamRoleArnParser; PolicyReaderOptions policyReaderOptions = new PolicyReaderOptions(); policyReaderOptions.setStripAwsPrincipalIdHyphensEnabled(false); @@ -82,11 +88,10 @@ public KmsPolicyService(@Named(ROOT_USER_ARN_PROPERTY) String rootUserArn, * access the KMS key. * * @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 policyJson, String iamRoleArn) { - return iamPrincipalCanAccessKey(policyJson, iamRoleArn) && cmsHasKeyDeletePermissions(policyJson); + public boolean isPolicyValid(String policyJson) { + return consumerPrincipalIsAnArnAndNotAnId(policyJson) && cmsHasKeyDeletePermissions(policyJson); } /** @@ -97,16 +102,17 @@ public boolean isPolicyValid(String policyJson, String iamRoleArn) { * 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) { + protected boolean consumerPrincipalIsAnArnAndNotAnId(String policyJson) { try { Policy policy = policyReader.createPolicyFromJsonString(policyJson); return policy.getStatements() .stream() .anyMatch(statement -> StringUtils.equals(statement.getId(), CERBERUS_CONSUMER_SID) && - statementAppliesToPrincipal(statement, iamPrincipalArn)); + statement.getPrincipals() + .stream() + .anyMatch(principal -> awsIamRoleArnParser.isRoleArn(principal.getId()))); } 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); diff --git a/src/main/java/com/nike/cerberus/service/KmsService.java b/src/main/java/com/nike/cerberus/service/KmsService.java index 036c80f79..bc3c7521c 100644 --- a/src/main/java/com/nike/cerberus/service/KmsService.java +++ b/src/main/java/com/nike/cerberus/service/KmsService.java @@ -217,7 +217,7 @@ public void validateKeyAndPolicy(AwsIamRoleKmsKeyRecord kmsKeyRecord, String iam try { String keyPolicy = getKmsKeyPolicy(awsKmsKeyArn, kmsCMKRegion); - if (!kmsPolicyService.isPolicyValid(keyPolicy, iamPrincipalArn)) { + if (!kmsPolicyService.isPolicyValid(keyPolicy)) { logger.info("The KMS key: {} generated for IAM principal: {} contained an invalid policy, regenerating", awsKmsKeyArn, iamPrincipalArn); diff --git a/src/test/java/com/nike/cerberus/service/KmsPolicyServiceTest.java b/src/test/java/com/nike/cerberus/service/KmsPolicyServiceTest.java index 25249c051..154af8580 100644 --- a/src/test/java/com/nike/cerberus/service/KmsPolicyServiceTest.java +++ b/src/test/java/com/nike/cerberus/service/KmsPolicyServiceTest.java @@ -6,6 +6,7 @@ import com.amazonaws.auth.policy.actions.KMSActions; import com.fasterxml.jackson.databind.JsonNode; import com.fasterxml.jackson.databind.ObjectMapper; +import com.nike.cerberus.util.AwsIamRoleArnParser; import org.apache.commons.io.IOUtils; import org.apache.commons.lang3.StringUtils; import org.assertj.core.util.Lists; @@ -36,7 +37,7 @@ public void before() { String rootUserArn = "arn:aws:iam::1111111111:root"; String adminRoleArn = "arn:aws:iam::1111111111:role/admin"; String cmsRoleArn = "arn:aws:iam::1111111111:role/cms-iam-role"; - kmsPolicyService = new KmsPolicyService(rootUserArn, adminRoleArn, cmsRoleArn); + kmsPolicyService = new KmsPolicyService(rootUserArn, adminRoleArn, cmsRoleArn, new AwsIamRoleArnParser()); objectMapper = new ObjectMapper(); } @@ -62,7 +63,7 @@ public void test_that_isPolicyValid_returns_true_with_a_valid_policy() throws IO .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)); + assertTrue(kmsPolicyService.isPolicyValid(policyJsonAsString)); policy.close(); } @@ -73,7 +74,7 @@ public void test_that_isPolicyValid_returns_false_with_an_invalid_policy() throw .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)); + assertFalse(kmsPolicyService.isPolicyValid(policyJsonAsString)); policy.close(); } @@ -84,14 +85,14 @@ public void test_that_isPolicyValid_returns_false_when_cms_does_not_have_delete_ .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)); + assertFalse(kmsPolicyService.isPolicyValid(policyJsonAsString)); policy.close(); } @Test public void test_that_isPolicyValid_returns_false_when_a_non_standard_policy_is_supplied() { - assertFalse(kmsPolicyService.isPolicyValid(null, CERBERUS_CONSUMER_IAM_ROLE_ARN)); + assertFalse(kmsPolicyService.isPolicyValid(null)); } @Test diff --git a/src/test/java/com/nike/cerberus/service/KmsServiceTest.java b/src/test/java/com/nike/cerberus/service/KmsServiceTest.java index b3ea4b262..abfa5b16b 100644 --- a/src/test/java/com/nike/cerberus/service/KmsServiceTest.java +++ b/src/test/java/com/nike/cerberus/service/KmsServiceTest.java @@ -137,7 +137,7 @@ public void test_validatePolicy_validates_policy_when_validate_interval_has_pass when(result.getPolicy()).thenReturn(policy); when(client.getKeyPolicy(new GetKeyPolicyRequest().withKeyId(kmsKeyArn) .withPolicyName("default"))).thenReturn(result); - when(kmsPolicyService.isPolicyValid(policy, kmsKeyArn)).thenReturn(true); + when(kmsPolicyService.isPolicyValid(policy)).thenReturn(true); AwsIamRoleKmsKeyRecord kmsKey = mock(AwsIamRoleKmsKeyRecord.class); when(kmsKey.getAwsIamRoleId()).thenReturn(awsIamRoleRecordId); @@ -151,7 +151,7 @@ public void test_validatePolicy_validates_policy_when_validate_interval_has_pass verify(client, times(1)).getKeyPolicy(new GetKeyPolicyRequest().withKeyId(kmsKeyArn) .withPolicyName("default")); - verify(kmsPolicyService, times(1)).isPolicyValid(policy, kmsKeyArn); + verify(kmsPolicyService, times(1)).isPolicyValid(policy); } @Test @@ -172,7 +172,7 @@ public void test_validateKeyAndPolicy_validates_policy_when_validate_interval_ha kmsService.validateKeyAndPolicy(kmsKey, iamPrincipalArn); verify(kmsClientFactory, never()).getClient(anyString()); - verify(kmsPolicyService, never()).isPolicyValid(anyString(), anyString()); + verify(kmsPolicyService, never()).isPolicyValid(anyString()); } @Test @@ -201,7 +201,7 @@ public void test_validateKeyAndPolicy_does_not_throw_error_when_cannot_validate( kmsService.validateKeyAndPolicy(kmsKey, iamPrincipalArn); - verify(kmsPolicyService, never()).isPolicyValid(policy, iamPrincipalArn); + verify(kmsPolicyService, never()).isPolicyValid(policy); verify(client, never()).putKeyPolicy(anyObject()); }